From 24b874058dd941d50c544687625e8004e0caff46 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Thu, 20 Aug 2026 12:06:16 -0500 Subject: [PATCH 1/4] Reference the Freeze Doctor contract as a package instead of copying it (BL-16719) Stage 1 of the migration, on its own branch so PR #8218 stays reviewable and green. Nothing is published: the package is restored from a local folder that build/pack-contract-locally.ps1 fills from a clone of the Doctor repo, so this branch can be built and reviewed before anything irreversible happens. What goes away: - The three copied contract files, 750 lines of them. They were hand-maintained duplicates of files in BloomBooks/bloom-freeze-doctor describing a wire format the two programs must agree about exactly, and they had already drifted. - build/check-freeze-doctor-contract.sh and its workflow. That check existed only to catch drift between the copies; with one definition there is nothing to drift. - The RobustFile pre-commit exemption. It was needed because the copies could not call RobustFile and still match the originals. The files have left this repo, so the shared quality gate is back to full strength with nothing excused from it. That was the exemption John had to approve; it is now moot. What arrives: one PackageReference with no transitive dependencies of its own, two usings, and temporary local-feed scaffolding (root NuGet.Config, the pack script, localpackages/) that all disappears when the package is published. The layout-pinning test stays, with its purpose rewritten, because that purpose changed. It no longer guards against two copies diverging - it pins the layout Bloom expects against the layout the referenced version actually has, so a package upgrade that moved an offset fails the build instead of silently making Bloom publish its health where the Doctor no longer looks. Full suite: 3153 passed, 13 skipped, 0 failed - the same as with the copies. --- .github/workflows/freeze-doctor-contract.yml | 44 -- .gitignore | 5 + NuGet.Config | 25 + build/check-csharp-robustfile.sh | 8 - build/check-freeze-doctor-contract.sh | 176 ------- build/pack-contract-locally.ps1 | 46 ++ localpackages/.gitkeep | 1 + src/BloomExe/BloomExe.csproj | 8 + src/BloomExe/FreezeDoctor/DoctorChannel.cs | 461 ------------------ src/BloomExe/FreezeDoctor/DoctorSession.cs | 259 ---------- src/BloomExe/FreezeDoctor/DoctorSignals.cs | 133 ----- .../FreezeDoctor/FreezeDoctorSupport.cs | 3 + .../DoctorChannelContractTests.cs | 20 +- 13 files changed, 101 insertions(+), 1088 deletions(-) delete mode 100644 .github/workflows/freeze-doctor-contract.yml create mode 100644 NuGet.Config delete mode 100644 build/check-freeze-doctor-contract.sh create mode 100644 build/pack-contract-locally.ps1 create mode 100644 localpackages/.gitkeep delete mode 100644 src/BloomExe/FreezeDoctor/DoctorChannel.cs delete mode 100644 src/BloomExe/FreezeDoctor/DoctorSession.cs delete mode 100644 src/BloomExe/FreezeDoctor/DoctorSignals.cs diff --git a/.github/workflows/freeze-doctor-contract.yml b/.github/workflows/freeze-doctor-contract.yml deleted file mode 100644 index e15445d7865f..000000000000 --- a/.github/workflows/freeze-doctor-contract.yml +++ /dev/null @@ -1,44 +0,0 @@ -name: Freeze Doctor contract - -# Checks that the three Freeze Doctor contract files under src/BloomExe/FreezeDoctor still match -# their source of truth in BloomBooks/bloom-freeze-doctor. See -# build/check-freeze-doctor-contract.sh for why that matters: they describe a wire format two -# separate programs have to agree about exactly, and when they disagree nothing fails loudly — -# Bloom writes one set of offsets, the Doctor reads another, and the reports look plausible and -# are wrong. -# -# DELETE THIS WORKFLOW when the contract ships as a NuGet package and those files leave this -# repo. It exists only because they are copies. See BL-16719. -# -# This job DOES check out the repo and run a script from it, unlike pr-automation.yml. That is -# safe and is not a departure from the note there: the concern in that file is specifically -# `pull_request_target`, which would hand secrets and a writable token to untrusted code. This -# uses plain `pull_request`, so a fork PR gets a read-only token and no secrets, which is the -# ordinary way to run a repo's own checks. -on: - pull_request: - # Only when something involved actually changes, so this adds no check to unrelated PRs. - paths: - - "src/BloomExe/FreezeDoctor/DoctorChannel.cs" - - "src/BloomExe/FreezeDoctor/DoctorSession.cs" - - "src/BloomExe/FreezeDoctor/DoctorSignals.cs" - - "build/check-freeze-doctor-contract.sh" - - ".github/workflows/freeze-doctor-contract.yml" - # So it can be run on demand — in particular after a change lands in the OTHER repo, which by - # definition does not touch any path above and so cannot trigger this by itself. - workflow_dispatch: - -jobs: - contract: - runs-on: ubuntu-latest - permissions: - contents: read - steps: - - uses: actions/checkout@v4 - - # No .NET, no node: the script needs only git and the usual shell tools. It takes a - # shallow clone of the Doctor's repo, which is public, so no token is involved. - # (Deliberately a clone rather than raw.githubusercontent.com, whose CDN served stale - # content for minutes after a push and made the check report drift that did not exist.) - - name: Compare the contract files with the Doctor's repo - run: sh build/check-freeze-doctor-contract.sh diff --git a/.gitignore b/.gitignore index f03b28b52e34..c0cfc68733e8 100644 --- a/.gitignore +++ b/.gitignore @@ -199,3 +199,8 @@ pr-comments*.md # run build/ci/watch-for-hung-bloom.ps1 locally). Never meant to be committed. bloom-diagnostics/ bloom-watchdog/ + +# Temporary, while BloomFreezeDoctor.Contract is not published: build/pack-contract-locally.ps1 +# drops the package here and the root NuGet.Config restores it from here. See BL-16719. +localpackages/*.nupkg +localpackages/*.snupkg diff --git a/NuGet.Config b/NuGet.Config new file mode 100644 index 000000000000..ae5fa98c70a0 --- /dev/null +++ b/NuGet.Config @@ -0,0 +1,25 @@ + + + + + + + diff --git a/build/check-csharp-robustfile.sh b/build/check-csharp-robustfile.sh index fbff268d9590..521a5b6a5d51 100644 --- a/build/check-csharp-robustfile.sh +++ b/build/check-csharp-robustfile.sh @@ -35,14 +35,6 @@ if [ -s $filesToCheck ]; then case "$file" in src/BloomExe/RobustFileIO.cs) continue;; src/BloomTests/*) continue;; - # The Freeze Doctor contract files are copies of files in BloomBooks/bloom-freeze-doctor and must - # stay byte-identical to them apart from the namespace, so they cannot use RobustFile - it does not - # exist in that repository. Exempting them is safe rather than merely convenient: every call in them - # is a best-effort diagnostic write already wrapped in a catch-everything, and the failure they would - # otherwise retry through simply means "no session file this time", which the callers are built to - # tolerate. Nothing a user's work depends on passes through these files. - src/BloomExe/FreezeDoctor/DoctorChannel.cs) continue;; - src/BloomExe/FreezeDoctor/DoctorSession.cs) continue;; esac if awk ' # Flag ordinary banned file APIs directly. diff --git a/build/check-freeze-doctor-contract.sh b/build/check-freeze-doctor-contract.sh deleted file mode 100644 index 0dcb6b89831d..000000000000 --- a/build/check-freeze-doctor-contract.sh +++ /dev/null @@ -1,176 +0,0 @@ -#!/bin/sh -# Guards the three Freeze Doctor contract files in src/BloomExe/FreezeDoctor against drifting away -# from their source of truth in BloomBooks/bloom-freeze-doctor. -# -# WHY THIS EXISTS. Those files describe a wire format — shared-memory offsets, kernel object names, a -# JSON schema — that two separate programs have to agree about exactly. They are maintained as copies, -# and when copies of 750 lines are kept in step by hand, they drift: these two already had, and a fix -# made in one repo had to be carried across the other by hand. Drift here fails silently and -# expensively. Bloom writes one set of offsets, the Doctor reads another, and the result is a stream -# of confident, wrong diagnostic reports that nobody can tell apart from real ones. -# -# THIS IS DELIBERATELY TEMPORARY. The real fix is to stop having copies: ship the contract as a NuGet -# package from the Doctor's repo and let Bloom consume it, at which point these files leave this repo -# and this script should be deleted along with them. See BL-16719. -# -# WHAT IT DOES NOT CHECK. Only that the copies match. The layout constants are pinned separately, by -# value, in a test in each repo — so a deliberate change that is mirrored correctly but wrong still -# has to get past those. This is the outer of two nets, not the only one. -# -# It also only compares against whatever the other repo currently says. A change made there and not -# mirrored here is caught by the next PR that touches these files, or by running this on demand — the -# workflow has a manual trigger for exactly that, since a change in the other repo cannot trigger a -# workflow in this one. - -cd "$(dirname "$0")/.." || exit 1 - -missing_dependencies= -for dependency in git tr grep; do - if ! command -v "$dependency" >/dev/null 2>&1; then - missing_dependencies="$missing_dependencies $dependency" - fi -done -if [ -n "$missing_dependencies" ]; then - echo "Missing required commands for build/check-freeze-doctor-contract.sh:$missing_dependencies" - exit 1 -fi - -TEMP_CLONE= -cleanup() { - [ -n "$TEMP_CLONE" ] && rm -rf "$TEMP_CLONE" -} -trap cleanup EXIT - -echo "Checking the Freeze Doctor contract files against BloomBooks/bloom-freeze-doctor." - -OURS_DIR="src/BloomExe/FreezeDoctor" -FILES="DoctorChannel.cs DoctorSession.cs DoctorSignals.cs" - -# Where the files live in the Doctor's repo. TWO candidates on purpose: they are being moved into -# their own project so the contract can be published as a package, and this check must not care which -# of the two repos merges first. Newest location first. -THEIRS_CANDIDATES="src/BloomFreezeDoctor.Contract src/BloomFreezeDoctor.Core/Contract" - -# Reports the first candidate subdirectory under $1 that actually CONTAINS the contract, or nothing. -# -# Testing for the files, not the directory, on purpose. Switching the Doctor's repo between branches -# leaves the directory of the branch you left behind whenever it holds build output (obj/ is -# gitignored, so git cannot remove the folder) — so a directory-existence test picked an empty -# BloomFreezeDoctor.Contract/ and reported all three files as drifted. A false alarm is the one thing -# this check must not produce, because a check that cries wolf gets switched off. -find_subpath() { - for candidate in $THEIRS_CANDIDATES; do - if [ -f "$1/$candidate/DoctorChannel.cs" ]; then - printf '%s' "$candidate" - return 0 - fi - done - return 1 -} - -# Prefer a local clone, so a developer can run this offline and against uncommitted work. -# BLOOM_FREEZE_DOCTOR_REPO overrides where that clone is. -if [ -n "$BLOOM_FREEZE_DOCTOR_REPO" ]; then - CLONE="$BLOOM_FREEZE_DOCTOR_REPO" - if ! find_subpath "$CLONE" >/dev/null; then - echo "BLOOM_FREEZE_DOCTOR_REPO is set to '$CLONE', but none of these is there:" - for candidate in $THEIRS_CANDIDATES; do echo " $CLONE/$candidate"; done - exit 1 - fi -elif [ -d "../bloom-freeze-doctor" ] && find_subpath "../bloom-freeze-doctor" >/dev/null; then - CLONE="../bloom-freeze-doctor" -else - # Otherwise take a shallow clone. Deliberately NOT raw.githubusercontent.com: that is behind a CDN - # which served the previous version of a file for minutes after a push, so this check reported drift - # that did not exist. A check that cries wolf gets switched off, which would be worse than not having - # it — and this one exists precisely to be trusted. - TEMP_CLONE=$(mktemp -d 2>/dev/null || mktemp -d -t fdcontract) - echo " (no local clone found; taking a shallow one)" - if ! git clone --depth 1 --quiet https://github.com/BloomBooks/bloom-freeze-doctor "$TEMP_CLONE" 2>/dev/null; then - echo "Could not clone BloomBooks/bloom-freeze-doctor, so the contract files cannot be compared." - echo "If this machine has no network, set BLOOM_FREEZE_DOCTOR_REPO to a local clone instead." - exit 1 - fi - CLONE="$TEMP_CLONE" -fi - -THEIRS_SUBPATH=$(find_subpath "$CLONE") -if [ -z "$THEIRS_SUBPATH" ]; then - echo "Found the Doctor's repo at $CLONE but not the contract files in any known place:" - for candidate in $THEIRS_CANDIDATES; do echo " $candidate"; done - echo "They have probably moved again. Add the new location to THEIRS_CANDIDATES." - exit 1 -fi -echo " (comparing against $CLONE/$THEIRS_SUBPATH)" - -# Normalising away two things we do NOT want to fail on: -# * the namespace declaration, which is meant to differ (Bloom.FreezeDoctor vs -# BloomFreezeDoctor.Contract) and is the one intentional difference between the copies; -# * all whitespace, because the two repos' formatters disagree about where to wrap long lines. -# Bloom's csharpier hook rewraps three lines in these files on every commit, so a byte comparison -# would fail permanently and immediately be ignored, which is worse than no check at all. -# Everything that carries meaning — offsets, names, constants, logic — still has to match. -# -# Whitespace is DELETED, not squeezed to a single space. Squeezing is not enough: csharpier's -# rewrapping inserts a newline after an opening bracket, which squeezes to a space the other copy -# does not have, so `foo( 0, ...)` and `foo(0, ...)` still compared unequal and the check failed on -# formatting alone. Deleting is safe here because none of the three files contains a string literal -# with a space in it — checked, and worth re-checking if that ever changes, since inside a literal a -# space would then be invisible to this comparison. -normalize() { - grep -v '^namespace ' | tr -d '[:space:]' -} - -failed= -missing= -for file in $FILES; do - ours="$OURS_DIR/$file" - if [ ! -f "$ours" ]; then - # If the file has gone, the package migration has probably happened. Say so rather than failing - # obscurely: this script is supposed to be deleted at that point. - missing="$missing $file" - continue - fi - - source_description="$CLONE/$THEIRS_SUBPATH/$file" - theirs_raw=$(cat "$source_description" 2>/dev/null) - - if [ -z "$theirs_raw" ]; then - echo " COULD NOT READ $source_description — skipping $file rather than guessing." - failed="$failed $file" - continue - fi - - a=$(normalize < "$ours") - b=$(printf '%s' "$theirs_raw" | normalize) - - if [ "$a" = "$b" ]; then - echo " ok $file" - else - echo " DIFFERS $file" - failed="$failed $file" - fi -done - -if [ -n "$missing" ]; then - echo "" - echo "These files are no longer in $OURS_DIR:$missing" - echo "If the contract has moved to a NuGet package, delete this script — it has done its job." - exit 1 -fi - -if [ -n "$failed" ]; then - echo "" - echo "The Freeze Doctor contract has drifted, in:$failed" - echo "" - echo "These files must describe the same wire format in both repos. Copy the changed file across so" - echo "the two agree, in whichever direction is correct, and bump DoctorChannelLayout.SchemaVersion" - echo "in BOTH repos if the layout itself changed. To see the difference:" - echo "" - echo " diff <(cat $OURS_DIR/) <(cat ../bloom-freeze-doctor/$THEIRS_SUBPATH/)" - echo "" - echo "Differences in line wrapping alone do not trigger this — the comparison ignores whitespace." - exit 1 -fi - -echo "The Freeze Doctor contract files agree with the Doctor's repo." diff --git a/build/pack-contract-locally.ps1 b/build/pack-contract-locally.ps1 new file mode 100644 index 000000000000..e296427424e3 --- /dev/null +++ b/build/pack-contract-locally.ps1 @@ -0,0 +1,46 @@ +# Builds BloomFreezeDoctor.Contract from a clone of the Doctor's repo into ./localpackages, so this +# branch can be built before the package is published anywhere. +# +# TEMPORARY. This exists only because nothing is published yet: a push to a public feed cannot be +# undone, so we wanted to review the shape of the change first. When the package is published for +# real, delete this script, ./localpackages, and the NuGet.Config at the repository root. See BL-16719. + +$ErrorActionPreference = "Stop" + +$repoRoot = Split-Path -Parent $PSScriptRoot +$output = Join-Path $repoRoot "localpackages" + +# Where the Doctor's repo is. Beside this one by default, which is how our checkouts are usually laid +# out; override with BLOOM_FREEZE_DOCTOR_REPO if yours is somewhere else. +$doctorRepo = if ($env:BLOOM_FREEZE_DOCTOR_REPO) { + $env:BLOOM_FREEZE_DOCTOR_REPO +} else { + Join-Path (Split-Path -Parent $repoRoot) "bloom-freeze-doctor" +} + +$project = Join-Path $doctorRepo "src\BloomFreezeDoctor.Contract\BloomFreezeDoctor.Contract.csproj" + +if (-not (Test-Path $project)) { + Write-Host "Could not find the contract project at:" -ForegroundColor Yellow + Write-Host " $project" + Write-Host "" + Write-Host "Clone it beside this repository:" + Write-Host " git clone https://github.com/BloomBooks/bloom-freeze-doctor" + Write-Host "" + Write-Host "or point BLOOM_FREEZE_DOCTOR_REPO at an existing clone. Note that the contract project" + Write-Host "only exists on that repo's contract-package branch until it is merged." + exit 1 +} + +New-Item -ItemType Directory -Force -Path $output | Out-Null + +Write-Host "Packing $project" +Write-Host " to $output" +dotnet pack $project --configuration Release --output $output +if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } + +Write-Host "" +Write-Host "Done. Now build Bloom as usual; it will restore the package from ./localpackages." +Write-Host "If you have built this branch before, you may need to clear the cached copy first:" +Write-Host " dotnet nuget locals http-cache --clear" +Write-Host " Remove-Item -Recurse -Force ~/.nuget/packages/bloomfreezedoctor.contract" diff --git a/localpackages/.gitkeep b/localpackages/.gitkeep new file mode 100644 index 000000000000..79e6756b20d1 --- /dev/null +++ b/localpackages/.gitkeep @@ -0,0 +1 @@ +# Keeps this folder present so NuGet does not complain about a missing source before you run build/pack-contract-locally.ps1. Temporary; see the NuGet.Config at the repo root. diff --git a/src/BloomExe/BloomExe.csproj b/src/BloomExe/BloomExe.csproj index 8d2f19c1f7c9..e8eae1e98714 100644 --- a/src/BloomExe/BloomExe.csproj +++ b/src/BloomExe/BloomExe.csproj @@ -234,6 +234,14 @@ + + diff --git a/src/BloomExe/FreezeDoctor/DoctorChannel.cs b/src/BloomExe/FreezeDoctor/DoctorChannel.cs deleted file mode 100644 index 368c51ec68a3..000000000000 --- a/src/BloomExe/FreezeDoctor/DoctorChannel.cs +++ /dev/null @@ -1,461 +0,0 @@ -// Explicit usings and an explicit nullable context, rather than relying on the project's settings. -// This file is copied into BloomDesktop, which has neither ImplicitUsings nor nullable enabled, so -// depending on them would mean the copy had to be edited on the way in — and a file that has to be -// edited on the way in is a file that will drift. -#nullable enable -using System; -using System.IO; -using System.IO.MemoryMappedFiles; -using System.Text; - -namespace Bloom.FreezeDoctor; - -// ===================================================================================================== -// THIS FILE IS THE CONTRACT BETWEEN BLOOM AND THE FREEZE DOCTOR, AND IT IS COPIED INTO BOTH REPOS. -// -// Source of truth: BloomBooks/bloom-freeze-doctor, src/BloomFreezeDoctor.Core/Contract/DoctorChannel.cs -// The two copies must agree on everything except the namespace. They are NOT byte-identical: the two -// repos' formatters disagree about where to wrap a few long lines, so the comparison ignores -// whitespace. Two nets catch a drift: build/check-freeze-doctor-contract.sh compares the copies on -// every PR that touches them, and each repo has a test pinning SchemaVersion and the field offsets by -// value. Both exist because a drift here fails silently — Bloom writes one set of offsets, the Doctor -// reads another, and the resulting reports are confident and wrong. -// -// ALL OF THAT IS A WORKAROUND FOR THESE BEING COPIES AT ALL. The intended end state is to publish this -// as a NuGet package from the Doctor's repo and delete Bloom's copies, at which point the script goes -// too. It is not done yet because the format is still settling and a publish round-trip on every edit -// would cost more than it saves; the moment to do it is the Doctor's first packaged release. -// -// Why shared memory rather than a pipe, a socket, or Bloom's own web server: the Doctor has to be able -// to read this when Bloom is wedged. A request/response channel needs Bloom to be well enough to -// answer, which is exactly what we cannot assume — and Bloom's HTTP server in particular can be -// deadlocked or starved of worker threads, which is one of the failures we are hunting. Reading a page -// of memory needs nothing from Bloom at all. -// -// A memory-mapped section also outlives the process that created it for as long as any handle stays -// open, so the Doctor, which holds one, can still read Bloom's final state after Bloom has gone. That -// is what makes the clean-exit flag here (rather than only on disk) useful. -// ===================================================================================================== - -/// -/// A consistent snapshot of what Bloom last published about itself. -/// -public sealed record DoctorChannelSnapshot -{ - /// Layout version Bloom wrote with. - public required int SchemaVersion { get; init; } - - /// The process this describes. - public required int ProcessId { get; init; } - - /// - /// How many times Bloom's UI-thread timer has fired. Its *staleness* is the freeze signal — see - /// . - /// - public required long UiTicks { get; init; } - - /// - /// How long since the UI thread last ticked. This is the only signal that catches a UI thread blocked - /// in a managed wait on an STA thread, where the window still answers messages and every outside - /// probe reports the application as healthy. - /// - public required TimeSpan UiHeartbeatAge { get; init; } - - /// How many times Bloom's background watchdog thread has ticked. - public required long WatchdogTicks { get; init; } - - /// - /// How long since the watchdog thread last ticked. Compare with : a stale - /// UI heartbeat with a healthy watchdog means the UI thread is blocked, while both stale means the - /// whole process is wedged (a GC that will not finish, or a suspended process). - /// - public required TimeSpan WatchdogHeartbeatAge { get; init; } - - /// What Bloom says it is doing, for the report's opening lines. - public required string Activity { get; init; } - - /// - /// How far shutdown has got, or 0 if it has not started. Lets a Bloom that dies mid-shutdown say - /// *where* it stopped rather than merely that it did. - /// - public required int ShutdownPhase { get; init; } - - /// True once Bloom has recorded that its shutdown ran to completion. - public required bool CleanExitRecorded { get; init; } - - /// - /// True if Bloom sees a debugger attached. Authoritative, unlike our outside guess, and the reason a - /// developer stopping their debugger never produces a report. - /// - public required bool DebuggerAttached { get; init; } - - /// - /// True while Bloom is deliberately busy — publishing, uploading, making a PDF. Raises the Doctor's - /// patience rather than silencing it. - /// - public required bool LongOperationInProgress { get; init; } - - /// Server worker threads currently doing work, when Bloom reports it. - public required int ServerBusyWorkers { get; init; } - - /// - /// Server worker threads currently blocked. Bloom already tracks this for its own deadlock - /// avoidance; surfacing it costs nothing and says a great deal about a frozen publish. - /// - public required int ServerBlockedWorkers { get; init; } -} - -/// -/// The layout of the shared page, and the names used to find it. Both the writer (in Bloom) and the -/// reader (in the Doctor) work from these constants, so there is one description of the format rather -/// than two. -/// -public static class DoctorChannelLayout -{ - /// - /// Bump this only for an incompatible change. The reader refuses anything it does not recognise - /// rather than misreading it, so an old Doctor meeting a new Bloom degrades to Tier A instead of - /// reporting rubbish. - /// - public const int SchemaVersion = 1; - - /// - /// One page is ample and keeps the whole record on a single page of memory. `Local\` scope, not - /// `Global\`: the Doctor cannot open processes in another Windows session anyway. - /// - public const int Size = 4096; - - /// The name for a given Bloom process. - public static string NameFor(int processId) => - $@"Local\BloomFreezeDoctor.v{SchemaVersion}.{processId}"; - - // The layout. Offsets are explicit rather than computed so a change is visible in a diff. - internal const int OffsetSchemaVersion = 0; - internal const int OffsetProcessId = 4; - - /// - /// A sequence number, incremented before and after every write. Odd means a write is in progress. - /// The reader takes it before and after reading and retries if it changed, which is what stops it - /// seeing half of one update and half of the next — mattering most for the strings, since a torn - /// activity name would be gibberish on a card. - /// - internal const int OffsetWriteSequence = 8; - - internal const int OffsetUiTicks = 16; - internal const int OffsetUiTimestamp = 24; - internal const int OffsetWatchdogTicks = 32; - internal const int OffsetWatchdogTimestamp = 40; - internal const int OffsetShutdownPhase = 48; - internal const int OffsetFlags = 52; - internal const int OffsetServerBusy = 56; - internal const int OffsetServerBlocked = 60; - internal const int OffsetActivity = 64; - - /// - /// How much room the activity string gets. Public because it is part of the contract a caller has to - /// respect: anything longer is truncated rather than allowed to run into the next field. - /// - public const int ActivityMaxBytes = 256; - - internal const int FlagCleanExitRecorded = 1 << 0; - internal const int FlagDebuggerAttached = 1 << 1; - internal const int FlagLongOperation = 1 << 2; -} - -/// -/// Reads what a Bloom has published. Used by the Doctor; harmless if Bloom is an older version that -/// publishes nothing, in which case simply returns false and the Doctor falls back -/// to watching from outside. -/// -public static class DoctorChannelReader -{ - /// - /// Reads a consistent snapshot, or returns false if this Bloom publishes no channel (an older - /// version), the schema is one we do not understand, or the data would not settle. - /// - public static bool TryRead(int processId, out DoctorChannelSnapshot? snapshot) - { - snapshot = null; - try - { - using var file = MemoryMappedFile.OpenExisting( - DoctorChannelLayout.NameFor(processId), - MemoryMappedFileRights.Read - ); - using var view = file.CreateViewAccessor( - 0, - DoctorChannelLayout.Size, - MemoryMappedFileAccess.Read - ); - - // Retry a few times: a write in progress is momentary, and giving up after three attempts - // is better than looping while Bloom is busy. - for (var attempt = 0; attempt < 3; attempt++) - { - var before = view.ReadInt64(DoctorChannelLayout.OffsetWriteSequence); - if (before % 2 != 0) - continue; // a write is in flight - - var candidate = ReadFields(view, processId); - var after = view.ReadInt64(DoctorChannelLayout.OffsetWriteSequence); - if (before != after) - continue; // it changed under us - - if (candidate == null) - return false; // schema we do not understand: better nothing than nonsense - snapshot = candidate; - return true; - } - } - catch (FileNotFoundException) - { - // No channel: an older Bloom, or one that has not got that far in startup. Expected. - } - catch (Exception) - { - // Anything else (permissions, a half-created section) also means "no channel". - } - return false; - } - - private static DoctorChannelSnapshot? ReadFields(MemoryMappedViewAccessor view, int processId) - { - var schema = view.ReadInt32(DoctorChannelLayout.OffsetSchemaVersion); - if (schema != DoctorChannelLayout.SchemaVersion) - return null; - - var now = Environment.TickCount64; - var flags = view.ReadInt32(DoctorChannelLayout.OffsetFlags); - var activityBytes = new byte[DoctorChannelLayout.ActivityMaxBytes]; - view.ReadArray(DoctorChannelLayout.OffsetActivity, activityBytes, 0, activityBytes.Length); - - return new DoctorChannelSnapshot - { - SchemaVersion = schema, - ProcessId = view.ReadInt32(DoctorChannelLayout.OffsetProcessId), - UiTicks = view.ReadInt64(DoctorChannelLayout.OffsetUiTicks), - // Both sides use Environment.TickCount64, which counts since the machine booted and is - // therefore directly comparable between processes — unlike a wall clock, which a time - // change or a sleep would skew. - UiHeartbeatAge = AgeOf(view.ReadInt64(DoctorChannelLayout.OffsetUiTimestamp), now), - WatchdogTicks = view.ReadInt64(DoctorChannelLayout.OffsetWatchdogTicks), - WatchdogHeartbeatAge = AgeOf( - view.ReadInt64(DoctorChannelLayout.OffsetWatchdogTimestamp), - now - ), - Activity = DecodeString(activityBytes), - ShutdownPhase = view.ReadInt32(DoctorChannelLayout.OffsetShutdownPhase), - CleanExitRecorded = (flags & DoctorChannelLayout.FlagCleanExitRecorded) != 0, - DebuggerAttached = (flags & DoctorChannelLayout.FlagDebuggerAttached) != 0, - LongOperationInProgress = (flags & DoctorChannelLayout.FlagLongOperation) != 0, - ServerBusyWorkers = view.ReadInt32(DoctorChannelLayout.OffsetServerBusy), - ServerBlockedWorkers = view.ReadInt32(DoctorChannelLayout.OffsetServerBlocked), - }; - } - - private static TimeSpan AgeOf(long timestamp, long now) => - timestamp <= 0 - ? TimeSpan.MaxValue - : TimeSpan.FromMilliseconds(Math.Max(0, now - timestamp)); - - private static string DecodeString(byte[] bytes) - { - var length = Array.IndexOf(bytes, (byte)0); - if (length < 0) - length = bytes.Length; - return length == 0 ? "" : Encoding.UTF8.GetString(bytes, 0, length); - } -} - -/// -/// Publishes Bloom's state into the shared page. Lives in Bloom; here in the Doctor's repo only so that -/// the two sides share one description of the format, and so the Doctor's own tests can write a channel -/// to read back. -/// -/// **Every method must be safe to call from anywhere and must never throw**, because the callers are -/// Bloom's UI thread and Bloom's shutdown path. Diagnostics that can break the application they -/// diagnose are worse than no diagnostics. -/// -public sealed class DoctorChannelWriter : IDisposable -{ - private readonly MemoryMappedFile? _file; - private readonly MemoryMappedViewAccessor? _view; - - /// - /// Serialises the whole of . - /// - /// This is not belt-and-braces: without it the sequence protocol is broken, and broken in a way that - /// silently disables the channel for the rest of the run. Two threads publish here — the UI-thread timer - /// and the watchdog thread — and `++_writeSequence` is a non-atomic read-modify-write, so a lost update - /// can leave the counter resting on an ODD value, which every reader interprets as "a write is in - /// progress" and gives up on, for ever. Even with an atomic counter, two overlapping writers can let a - /// reader see an unchanged even sequence around a half-written state, which is precisely the torn read - /// the sequence exists to prevent. - /// - /// A private lock cannot deadlock anything we are diagnosing: only these two diagnostic callers ever take - /// it, neither holds another lock, and the critical section is a handful of writes to a resident page. - /// - private readonly object _writeLock = new(); - - private long _writeSequence; - private long _uiTicks; - private long _watchdogTicks; - - /// - /// Creates the channel for this process. If it cannot be created, every method afterwards does - /// nothing: publishing diagnostics is never worth failing a startup over. - /// - public DoctorChannelWriter(int processId) - { - try - { - _file = MemoryMappedFile.CreateNew( - DoctorChannelLayout.NameFor(processId), - DoctorChannelLayout.Size, - MemoryMappedFileAccess.ReadWrite - ); - _view = _file.CreateViewAccessor( - 0, - DoctorChannelLayout.Size, - MemoryMappedFileAccess.ReadWrite - ); - _view.Write(DoctorChannelLayout.OffsetSchemaVersion, DoctorChannelLayout.SchemaVersion); - _view.Write(DoctorChannelLayout.OffsetProcessId, processId); - } - catch (Exception) - { - _file = null; - _view = null; - } - } - - /// True if the channel was created and is being published. - public bool IsOpen => _view != null; - - /// - /// Records that the UI thread is alive. Call from a UI-thread timer; the Doctor watches how long ago - /// this last happened. - /// - public void RecordUiTick() => - Write(view => - { - view.Write(DoctorChannelLayout.OffsetUiTicks, ++_uiTicks); - view.Write(DoctorChannelLayout.OffsetUiTimestamp, Environment.TickCount64); - }); - - /// - /// Records that the background watchdog thread is alive, which is how the Doctor distinguishes "the - /// UI thread is blocked" from "the whole process is wedged". - /// - public void RecordWatchdogTick() => - Write(view => - { - view.Write(DoctorChannelLayout.OffsetWatchdogTicks, ++_watchdogTicks); - view.Write(DoctorChannelLayout.OffsetWatchdogTimestamp, Environment.TickCount64); - }); - - /// Says what Bloom is doing, in words fit for a bug report. - public void SetActivity(string activity) => - Write(view => - { - var bytes = new byte[DoctorChannelLayout.ActivityMaxBytes]; - var encoded = Encoding.UTF8.GetBytes(activity ?? ""); - var length = Math.Min(encoded.Length, bytes.Length - 1); - // Truncate on a character boundary. Activity text can carry a book title or a file path, so - // cutting mid-sequence through a multi-byte character would leave the reader decoding a broken - // byte — and the report quoting a mangled name. Only when we actually truncated: `encoded[length]` - // is past the end otherwise, and the resulting exception used to leave the write sequence odd for - // ever, which silently disabled the whole channel. - if (length < encoded.Length) - { - while (length > 0 && (encoded[length] & 0xC0) == 0x80) - length--; - } - Array.Copy(encoded, bytes, length); - view.WriteArray(DoctorChannelLayout.OffsetActivity, bytes, 0, bytes.Length); - }); - - /// Marks a deliberately long operation, which buys Bloom patience rather than silence. - public void SetLongOperation(bool inProgress) => - SetFlag(DoctorChannelLayout.FlagLongOperation, inProgress); - - /// Publishes whether a debugger is attached, which is authoritative and stops false reports. - public void SetDebuggerAttached(bool attached) => - SetFlag(DoctorChannelLayout.FlagDebuggerAttached, attached); - - /// Records how far shutdown has got. - public void SetShutdownPhase(int phase) => - Write(view => view.Write(DoctorChannelLayout.OffsetShutdownPhase, phase)); - - /// Records that shutdown ran to completion — the proof whose absence is itself evidence. - public void RecordCleanExit() => SetFlag(DoctorChannelLayout.FlagCleanExitRecorded, true); - - /// Publishes the server's worker counts, which say a great deal about a frozen publish. - public void SetServerWorkerCounts(int busy, int blocked) => - Write(view => - { - view.Write(DoctorChannelLayout.OffsetServerBusy, busy); - view.Write(DoctorChannelLayout.OffsetServerBlocked, blocked); - }); - - private void SetFlag(int flag, bool value) => - Write(view => - { - var flags = view.ReadInt32(DoctorChannelLayout.OffsetFlags); - flags = value ? flags | flag : flags & ~flag; - view.Write(DoctorChannelLayout.OffsetFlags, flags); - }); - - /// - /// Performs one update between two increments of the sequence number, so a reader can tell whether - /// it saw a settled state. Swallows everything: see the class comment. - /// - private void Write(Action update) - { - var view = _view; - if (view == null) - return; - try - { - lock (_writeLock) - { - // EVERY increment is inside this try, and the finally restores parity from whatever value we - // actually reached. That ordering is the whole point, and getting it wrong here is unusually - // expensive: an odd resting value means "a write is in progress" to every reader, for ever, so - // the channel silently disables itself for the rest of the run and the Doctor falls back to - // watching from outside with no indication why. - // - // The earlier version incremented the counter in the same statement that wrote it, from - // outside the inner try. A throw from that one write then left the counter odd with no finally - // to correct it — and from then on every write published an even value while in progress and - // came to rest on an odd one, which is exactly backwards. Letting a reader see one - // inconsistent state is a far smaller loss than that. - try - { - _writeSequence++; // now odd: a write is in progress - view.Write(DoctorChannelLayout.OffsetWriteSequence, _writeSequence); - update(view); - } - finally - { - if (_writeSequence % 2 != 0) - _writeSequence++; - view.Write(DoctorChannelLayout.OffsetWriteSequence, _writeSequence); - } - } - } - catch (Exception) - { - // Never let publishing state break the thing whose state we are publishing. Note that the - // counter itself is even by now whatever happened above, so even a failure that stopped the - // final view.Write is repaired by the next successful write rather than lasting for ever. - } - } - - /// - public void Dispose() - { - _view?.Dispose(); - _file?.Dispose(); - } -} diff --git a/src/BloomExe/FreezeDoctor/DoctorSession.cs b/src/BloomExe/FreezeDoctor/DoctorSession.cs deleted file mode 100644 index 4690b11078f4..000000000000 --- a/src/BloomExe/FreezeDoctor/DoctorSession.cs +++ /dev/null @@ -1,259 +0,0 @@ -// Explicit usings and nullable context: this file is copied into BloomDesktop, which has neither -// ImplicitUsings nor nullable enabled. See DoctorChannel.cs for the full explanation. -#nullable enable -using System; -using System.Collections.Generic; -using System.IO; -using System.Text.Json; -using System.Text.Json.Serialization; - -namespace Bloom.FreezeDoctor; - -// ===================================================================================================== -// SECOND HALF OF THE CONTRACT BETWEEN BLOOM AND THE FREEZE DOCTOR. COPIED INTO BOTH REPOS. -// -// Source of truth: BloomBooks/bloom-freeze-doctor, src/BloomFreezeDoctor.Core/Contract/DoctorSession.cs -// -// DoctorChannel.cs carries what changes moment to moment, in shared memory. This file carries the facts -// that do not change, and — crucially — the ones that must OUTLIVE the process. Shared memory dies when -// the last handle closes, so a Bloom that crashes while no Doctor is watching leaves nothing behind. A -// file survives a crash, a Doctor restart, and a reboot. -// -// Everything here also removes a guess the Doctor would otherwise have to make from outside. Two are -// worth naming, because both were measured as getting it wrong: -// -// * WHICH LOG IS THIS BLOOM'S. Bloom recreates Log.txt every run and falls back to a randomly-named -// Log-tmpXXXX.txt only when another Bloom already holds it — so in the restart-after-a-freeze case, -// the frozen Bloom owns Log.txt and the healthy new one owns the tmp file, and "newest file wins" -// picks the wrong one. Bloom simply telling us the path retires that whole problem. -// * WHICH DEBUG PORT. The arithmetic differs by Bloom version, Bloom's own HTTP port belongs to http.sys -// rather than to Bloom, and a machine running two Blooms can hand the Doctor the wrong browser. -// ===================================================================================================== - -/// -/// What Bloom records about itself when it starts, for any Doctor that comes looking — including one -/// installed after the fact, or started after Bloom has already died. -/// -public sealed record DoctorSession -{ - /// Layout version, so a newer Doctor can read an older machine's files. - public int SchemaVersion { get; init; } = DoctorSessionStore.SchemaVersion; - - /// The process this describes. - public int ProcessId { get; init; } - - /// When it started, which is how a log file is matched to it. - public DateTimeOffset StartedAtUtc { get; init; } - - /// Full path to Bloom's executable. - public string ExePath { get; init; } = ""; - - /// Bloom's version. - public string Version { get; init; } = ""; - - /// Release channel: Release, Beta, Developer/Debug… - public string Channel { get; init; } = ""; - - /// The command line, which reveals an automation or headless run. - public string CommandLine { get; init; } = ""; - - /// - /// The log file Bloom is actually writing to. The single most valuable field here: see the note at the - /// top of this file about why guessing it from outside is systematically wrong. - /// - public string LogPath { get; init; } = ""; - - /// Bloom's own HTTP port, which cannot be discovered from outside because http.sys owns it. - public int HttpPort { get; init; } - - /// The WebView2 debugging port, so the Doctor need not infer it. - public int CdpPort { get; init; } - - /// The collection in use, for the report's context. - public string CollectionName { get; init; } = ""; - - /// - /// True when Bloom's own reporting has already told us about a problem this run — a Sentry event or a - /// tracker card. The Doctor defers to it rather than filing a second report about the same thing. - /// - /// This lives on the session rather than inside , and that placement is the whole - /// point: a user can file a problem report and then carry on working for hours. Recording it as an exit - /// would describe a running Bloom as finished, which then reads as proof of an orderly shutdown for a - /// process that may go on to crash. - /// - public bool BloomAlreadyReported { get; init; } - - /// The card or event Bloom filed, if it filed one. - public string? ReportedId { get; init; } - - /// - /// How this run ended, once it has. Null while Bloom is running — and null *after* Bloom has gone is - /// itself the evidence that it did not shut down properly. Nothing may set this while Bloom is still - /// running; see for what used to get that wrong. - /// - public DoctorSessionExit? Exit { get; init; } -} - -/// How a Bloom run ended, written on the way out. -public sealed record DoctorSessionExit -{ - /// When it ended. - public DateTimeOffset AtUtc { get; init; } - - /// How far shutdown got. See Bloom's Program.Run for what the numbers mean. - public int ShutdownPhase { get; init; } - - /// - /// True when this exit was forced by the Doctor asking Bloom to go, rather than being an orderly - /// shutdown. Recorded so that ending a zombie is not later mistaken for proof that it shut down properly. - /// - public bool ForcedByDoctor { get; init; } -} - -/// -/// Reads and writes the session files. Both sides use this, so there is one description of where the -/// files live and how they are named. -/// -public static class DoctorSessionStore -{ - /// Bump only for an incompatible change; readers ignore versions they do not know. - public const int SchemaVersion = 1; - - /// - /// Where the files live. Under the user's own local application data, because that is writable - /// without any privilege and is per-user, which matches who can watch whose processes anyway. - /// - public static string DefaultDirectory => - Path.Combine( - Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData), - "SIL", - "BloomFreezeDoctor", - "sessions" - ); - - /// - /// The file for one process. - /// - /// Keyed by process id, which Windows reuses — so a reader that finds a file for a *live* pid must check - /// against that process's actual start time before believing the - /// file describes it. The alternative (a unique id in the name) would stop a Doctor finding the file for a - /// pid it is watching, which is the common case this has to be good at. - /// - public static string PathFor(int processId, string? directory = null) => - Path.Combine(directory ?? DefaultDirectory, $"bloom-{processId}.json"); - - private static readonly JsonSerializerOptions Options = new() - { - WriteIndented = true, - DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, - }; - - /// - /// Writes a session file, temp-then-rename so that a reader never sees half a JSON document. Returns - /// false rather than throwing: Bloom must not fail because a diagnostic file could not be written. - /// - public static bool TryWrite(DoctorSession session, string? directory = null) - { - try - { - var path = PathFor(session.ProcessId, directory); - Directory.CreateDirectory(Path.GetDirectoryName(path)!); - var temp = path + ".tmp"; - File.WriteAllText(temp, JsonSerializer.Serialize(session, Options)); - // A rename within one volume is atomic; a copy is not, and this file is rewritten repeatedly. - File.Move(temp, path, overwrite: true); - return true; - } - catch (Exception) - { - return false; - } - } - - /// Reads one session file, or null if it is absent or unreadable. - public static DoctorSession? TryRead(int processId, string? directory = null) - { - try - { - var path = PathFor(processId, directory); - if (!File.Exists(path)) - return null; - var session = JsonSerializer.Deserialize( - File.ReadAllText(path), - Options - ); - // Refuse a schema we do not understand rather than misreading it. - return session != null && session.SchemaVersion == SchemaVersion ? session : null; - } - catch (Exception) - { - return null; - } - } - - /// - /// Every session file on the machine, for a Doctor that has just started and wants to know what has - /// been happening — including sessions whose processes are long gone, which is how an unreported crash - /// is discovered after the fact. - /// - public static List ReadAll(string? directory = null) - { - var sessions = new List(); - try - { - var folder = directory ?? DefaultDirectory; - if (!Directory.Exists(folder)) - return sessions; - foreach (var path in Directory.GetFiles(folder, "bloom-*.json")) - { - try - { - var session = JsonSerializer.Deserialize( - File.ReadAllText(path), - Options - ); - if (session != null && session.SchemaVersion == SchemaVersion) - sessions.Add(session); - } - catch (Exception) - { - // One unreadable file must not hide the rest. - } - } - } - catch (Exception) { } - return sessions; - } - - /// - /// Deletes session files that are of no further interest: their process is gone and either they - /// recorded a clean exit or they are older than the cutoff. Keeping an unexplained exit around is the - /// point, so those survive until they age out. - /// - public static void Prune( - Func processIsAlive, - TimeSpan maxAge, - string? directory = null - ) - { - foreach (var session in ReadAll(directory)) - { - try - { - if (processIsAlive(session.ProcessId)) - continue; - var tooOld = DateTimeOffset.UtcNow - session.StartedAtUtc > maxAge; - // "Explained" means an ORDERLY exit. An exit that was forced — a hard failure, or the Doctor - // ending a zombie — is not an explanation, it is the evidence; deleting it early would throw - // away the record of the very thing we exist to report. - var explained = session.Exit != null && !session.Exit.ForcedByDoctor; - if (tooOld || explained) - File.Delete(PathFor(session.ProcessId, directory)); - } - catch (Exception) - { - // Locked or already gone; the next pass will deal with it. - } - } - } -} diff --git a/src/BloomExe/FreezeDoctor/DoctorSignals.cs b/src/BloomExe/FreezeDoctor/DoctorSignals.cs deleted file mode 100644 index a55de26308fe..000000000000 --- a/src/BloomExe/FreezeDoctor/DoctorSignals.cs +++ /dev/null @@ -1,133 +0,0 @@ -// Explicit usings and nullable context: copied into BloomDesktop. See DoctorChannel.cs. -#nullable enable -using System; -using System.Threading; - -namespace Bloom.FreezeDoctor; - -// ===================================================================================================== -// THIRD PART OF THE CONTRACT BETWEEN BLOOM AND THE FREEZE DOCTOR. COPIED INTO BOTH REPOS. -// -// Source of truth: BloomBooks/bloom-freeze-doctor, src/BloomFreezeDoctor.Core/Contract/DoctorSignals.cs -// -// Shared memory lets the Doctor watch. These named events let the two actually ask each other for -// something, in the two cases where waiting is worth it: -// -// * ENDING A ZOMBIE. When Bloom's UI is gone but the process lives on, the Doctor can ask Bloom to exit -// under its own power. That is much better than killing it from outside: Bloom's ProcessExit runs, so -// its single-instance token is released properly and its own clean-exit record is written. Killing is -// the fallback for when nobody is listening. -// -// * DUMPING A DYING BLOOM. A crash gives us one short window in which the process still exists. Bloom -// signals, the Doctor dumps it from outside, and Bloom waits briefly. Dumping from outside beats -// self-dumping a process whose state is already suspect. -// -// Everything here is built so that the ABSENCE of the other side costs nothing. Bloom never waits unless -// it has first confirmed, with a zero timeout, that a Doctor is actually watching — because an -// unconditional pause would make every crash worse for the majority of users, who have no Doctor -// installed. -// ===================================================================================================== - -/// -/// The named events Bloom and the Doctor use to ask each other for something. All in the `Local\` -/// namespace, since neither can act on the other across Windows sessions anyway. -/// -public static class DoctorSignals -{ - /// - /// Created by the Doctor while it is watching a particular Bloom. Bloom tests for it with a zero - /// timeout before it ever agrees to wait for anything: no Doctor, no waiting. - /// - public static string WatchingName(int processId) => - $@"Local\BloomFreezeDoctor.watching.{processId}"; - - /// - /// Set by the Doctor to ask Bloom to exit under its own power. Bloom's watchdog thread waits on this, - /// which is the point: that thread is still running even when the UI thread is long gone. - /// - public static string QuitRequestName(int processId) => - $@"Local\BloomFreezeDoctor.quit.{processId}"; - - /// Set by Bloom as it dies, to ask for a dump while the process still exists. - public static string DumpRequestName(int processId) => - $@"Local\BloomFreezeDoctor.dumpme.{processId}"; - - /// Set by the Doctor when the dump is written, so Bloom can stop waiting. - public static string DumpCompleteName(int processId) => - $@"Local\BloomFreezeDoctor.dumped.{processId}"; - - /// - /// Creates (or opens) a manual-reset event by name, or returns null if that is not possible. Never - /// throws: a signal we cannot create simply means that capability is unavailable, and both sides are - /// written to carry on without it. - /// - public static EventWaitHandle? TryCreate(string name) - { - try - { - return new EventWaitHandle(false, EventResetMode.ManualReset, name); - } - catch (Exception) - { - return null; - } - } - - /// Opens an existing event, or null if nobody has created it. - public static EventWaitHandle? TryOpen(string name) - { - try - { - return EventWaitHandle.OpenExisting(name); - } - catch (Exception) - { - // WaitHandleCannotBeOpenedException in the ordinary case: the other side is not there. - return null; - } - } - - /// - /// True if the named event exists. Used for the question "is a Doctor watching me?", which must be - /// answerable instantly and without waiting. - /// - public static bool Exists(string name) - { - using var handle = TryOpen(name); - return handle != null; - } - - /// Sets an existing event, if there is one. Returns whether anyone was listening. - public static bool TrySignal(string name) - { - try - { - using var handle = TryOpen(name); - if (handle == null) - return false; - handle.Set(); - return true; - } - catch (Exception) - { - return false; - } - } - - /// - /// Waits for an event to be set, up to a limit. Returns false if it was not set in time, or could not - /// be opened at all — the caller treats both the same way, by carrying on. - /// - public static bool WaitFor(string name, TimeSpan timeout) - { - try - { - using var handle = TryOpen(name); - return handle != null && handle.WaitOne(timeout); - } - catch (Exception) - { - return false; - } - } -} diff --git a/src/BloomExe/FreezeDoctor/FreezeDoctorSupport.cs b/src/BloomExe/FreezeDoctor/FreezeDoctorSupport.cs index 1fc2c1a1be31..994eb4eb6b51 100644 --- a/src/BloomExe/FreezeDoctor/FreezeDoctorSupport.cs +++ b/src/BloomExe/FreezeDoctor/FreezeDoctorSupport.cs @@ -2,6 +2,9 @@ using System.Diagnostics; using System.Threading; using System.Windows.Forms; +// The wire format Bloom shares with the Freeze Doctor, from the package the Doctor's repo publishes. +// This used to be three files copied into this folder; see BL-16719 for why it is a package now. +using BloomFreezeDoctor.Contract; using SIL.Reporting; namespace Bloom.FreezeDoctor diff --git a/src/BloomTests/FreezeDoctor/DoctorChannelContractTests.cs b/src/BloomTests/FreezeDoctor/DoctorChannelContractTests.cs index a5d230a55db2..8f8714f91e24 100644 --- a/src/BloomTests/FreezeDoctor/DoctorChannelContractTests.cs +++ b/src/BloomTests/FreezeDoctor/DoctorChannelContractTests.cs @@ -1,5 +1,6 @@ using System; using Bloom.FreezeDoctor; +using BloomFreezeDoctor.Contract; using NUnit.Framework; namespace BloomTests.FreezeDoctor @@ -8,13 +9,18 @@ namespace BloomTests.FreezeDoctor /// Pins the shared-memory contract Bloom publishes for the Bloom Freeze Doctor /// (https://github.com/BloomBooks/bloom-freeze-doctor). /// - /// **The point of this fixture is to make drift break a build.** `DoctorChannel.cs` is a copy of a - /// file in the Doctor's repository, and the two must agree byte for byte about the layout. If they - /// ever disagree, nothing fails loudly: Bloom writes one set of offsets, the Doctor reads another, and - /// the result is a stream of reports full of plausible nonsense that nobody can tell from real ones. - /// So the schema version, the page size and the name format are asserted here BY VALUE, and the - /// equivalent fixture in the Doctor's repository asserts the same numbers. Changing the layout should - /// therefore require changing two tests in two repositories, which is the intended amount of friction. + /// **What this fixture is for changed when the contract became a package.** It used to guard against + /// two hand-maintained copies of the same file drifting apart. There is only one copy now — the + /// `BloomFreezeDoctor.Contract` package — so drift in that sense is no longer possible. + /// + /// It still earns its place, for a different reason: it pins the layout Bloom *expects* against the + /// layout the referenced package version actually has. A package upgrade that changed an offset or + /// the schema version would otherwise be silent — Bloom would compile, run, and publish its health to + /// offsets the Doctor no longer reads, and the reports would be plausible nonsense that nobody could + /// tell from real ones. Asserting the numbers BY VALUE here turns that into a failed build. + /// + /// So: if this fails after a version bump, the layout changed, and Bloom's side needs looking at + /// rather than the numbers here being updated to match. /// [TestFixture] public class DoctorChannelContractTests From d9e98321d38c0032b19f8e216744f13cfda51e21 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Thu, 20 Aug 2026 12:58:49 -0500 Subject: [PATCH 2/4] Rename the shared protocol to BloomBooks.FreezeDoctor.Protocol (BL-16719) The three files Bloom and the Freeze Doctor share are not a "contract" in any sense a reader would recognize from the word; they are the wire format of the shared-memory channel between the two processes - a protocol. So the package, its namespace, its folder in the Doctor's repo, and everything here that names it, all say Protocol now. On this side that means: - BloomExe.csproj references BloomBooks.FreezeDoctor.Protocol. - FreezeDoctorSupport.cs imports that namespace. - The local restore folder, NuGet.Config source key, and .gitignore comment name the new package. - build/pack-contract-locally.ps1 becomes build/pack-freeze-doctor-protocol.ps1 and packs the renamed project. - DoctorChannelContractTests.cs becomes FreezeDoctorProtocolTests.cs. Its purpose already changed when the protocol became a package - it no longer guards two hand-maintained copies against drift, because there is only one definition now; it pins the layout Bloom expects against the layout the referenced package version actually has, so a version bump that moved an offset fails the build instead of publishing plausible nonsense. No behavior changes: the assembly is byte-for-byte the same code under a different name. Full C# suite green (3153 passed, 13 skipped). The matching rename in the Doctor's repo is BloomBooks/bloom-freeze-doctor commit 8f569d9. Co-Authored-By: Claude Opus 5 (1M context) --- .gitignore | 5 +++-- NuGet.Config | 15 ++++++++------- ...lly.ps1 => pack-freeze-doctor-protocol.ps1} | 18 +++++++++--------- localpackages/.gitkeep | 2 +- src/BloomExe/BloomExe.csproj | 4 ++-- .../FreezeDoctor/FreezeDoctorSupport.cs | 2 +- ...ctTests.cs => FreezeDoctorProtocolTests.cs} | 15 ++++++++------- 7 files changed, 32 insertions(+), 29 deletions(-) rename build/{pack-contract-locally.ps1 => pack-freeze-doctor-protocol.ps1} (69%) rename src/BloomTests/FreezeDoctor/{DoctorChannelContractTests.cs => FreezeDoctorProtocolTests.cs} (95%) diff --git a/.gitignore b/.gitignore index c0cfc68733e8..e908888e5425 100644 --- a/.gitignore +++ b/.gitignore @@ -200,7 +200,8 @@ pr-comments*.md bloom-diagnostics/ bloom-watchdog/ -# Temporary, while BloomFreezeDoctor.Contract is not published: build/pack-contract-locally.ps1 -# drops the package here and the root NuGet.Config restores it from here. See BL-16719. +# Temporary, while BloomBooks.FreezeDoctor.Protocol is not published: +# build/pack-freeze-doctor-protocol.ps1 drops the package here and the root NuGet.Config restores it +# from here. See BL-16719. localpackages/*.nupkg localpackages/*.snupkg diff --git a/NuGet.Config b/NuGet.Config index ae5fa98c70a0..f0ce80fae330 100644 --- a/NuGet.Config +++ b/NuGet.Config @@ -2,14 +2,15 @@ - + diff --git a/build/pack-contract-locally.ps1 b/build/pack-freeze-doctor-protocol.ps1 similarity index 69% rename from build/pack-contract-locally.ps1 rename to build/pack-freeze-doctor-protocol.ps1 index e296427424e3..ce336c730304 100644 --- a/build/pack-contract-locally.ps1 +++ b/build/pack-freeze-doctor-protocol.ps1 @@ -1,9 +1,9 @@ -# Builds BloomFreezeDoctor.Contract from a clone of the Doctor's repo into ./localpackages, so this -# branch can be built before the package is published anywhere. +# Builds BloomBooks.FreezeDoctor.Protocol from a clone of the Doctor's repo into ./localpackages, so +# this branch can be built before the package is published anywhere. # -# TEMPORARY. This exists only because nothing is published yet: a push to a public feed cannot be -# undone, so we wanted to review the shape of the change first. When the package is published for -# real, delete this script, ./localpackages, and the NuGet.Config at the repository root. See BL-16719. +# TEMPORARY. This exists only because nothing is published yet: a push to nuget.org cannot be undone, +# so we wanted to review the shape of the change first. When the package is published for real, delete +# this script, ./localpackages, and the NuGet.Config at the repository root. See BL-16719. $ErrorActionPreference = "Stop" @@ -18,16 +18,16 @@ $doctorRepo = if ($env:BLOOM_FREEZE_DOCTOR_REPO) { Join-Path (Split-Path -Parent $repoRoot) "bloom-freeze-doctor" } -$project = Join-Path $doctorRepo "src\BloomFreezeDoctor.Contract\BloomFreezeDoctor.Contract.csproj" +$project = Join-Path $doctorRepo "src\BloomBooks.FreezeDoctor.Protocol\BloomBooks.FreezeDoctor.Protocol.csproj" if (-not (Test-Path $project)) { - Write-Host "Could not find the contract project at:" -ForegroundColor Yellow + Write-Host "Could not find the protocol project at:" -ForegroundColor Yellow Write-Host " $project" Write-Host "" Write-Host "Clone it beside this repository:" Write-Host " git clone https://github.com/BloomBooks/bloom-freeze-doctor" Write-Host "" - Write-Host "or point BLOOM_FREEZE_DOCTOR_REPO at an existing clone. Note that the contract project" + Write-Host "or point BLOOM_FREEZE_DOCTOR_REPO at an existing clone. Note that the protocol project" Write-Host "only exists on that repo's contract-package branch until it is merged." exit 1 } @@ -43,4 +43,4 @@ Write-Host "" Write-Host "Done. Now build Bloom as usual; it will restore the package from ./localpackages." Write-Host "If you have built this branch before, you may need to clear the cached copy first:" Write-Host " dotnet nuget locals http-cache --clear" -Write-Host " Remove-Item -Recurse -Force ~/.nuget/packages/bloomfreezedoctor.contract" +Write-Host " Remove-Item -Recurse -Force ~/.nuget/packages/bloombooks.freezedoctor.protocol" diff --git a/localpackages/.gitkeep b/localpackages/.gitkeep index 79e6756b20d1..da6c6507ac5e 100644 --- a/localpackages/.gitkeep +++ b/localpackages/.gitkeep @@ -1 +1 @@ -# Keeps this folder present so NuGet does not complain about a missing source before you run build/pack-contract-locally.ps1. Temporary; see the NuGet.Config at the repo root. +# Keeps this folder present so NuGet does not complain about a missing source before you run build/pack-freeze-doctor-protocol.ps1. Temporary; see the NuGet.Config at the repo root. diff --git a/src/BloomExe/BloomExe.csproj b/src/BloomExe/BloomExe.csproj index e8eae1e98714..21f81ca931d3 100644 --- a/src/BloomExe/BloomExe.csproj +++ b/src/BloomExe/BloomExe.csproj @@ -235,13 +235,13 @@ - + diff --git a/src/BloomExe/FreezeDoctor/FreezeDoctorSupport.cs b/src/BloomExe/FreezeDoctor/FreezeDoctorSupport.cs index 994eb4eb6b51..abe5701ad4bf 100644 --- a/src/BloomExe/FreezeDoctor/FreezeDoctorSupport.cs +++ b/src/BloomExe/FreezeDoctor/FreezeDoctorSupport.cs @@ -4,7 +4,7 @@ using System.Windows.Forms; // The wire format Bloom shares with the Freeze Doctor, from the package the Doctor's repo publishes. // This used to be three files copied into this folder; see BL-16719 for why it is a package now. -using BloomFreezeDoctor.Contract; +using BloomBooks.FreezeDoctor.Protocol; using SIL.Reporting; namespace Bloom.FreezeDoctor diff --git a/src/BloomTests/FreezeDoctor/DoctorChannelContractTests.cs b/src/BloomTests/FreezeDoctor/FreezeDoctorProtocolTests.cs similarity index 95% rename from src/BloomTests/FreezeDoctor/DoctorChannelContractTests.cs rename to src/BloomTests/FreezeDoctor/FreezeDoctorProtocolTests.cs index 8f8714f91e24..47a30483e16e 100644 --- a/src/BloomTests/FreezeDoctor/DoctorChannelContractTests.cs +++ b/src/BloomTests/FreezeDoctor/FreezeDoctorProtocolTests.cs @@ -1,17 +1,18 @@ using System; using Bloom.FreezeDoctor; -using BloomFreezeDoctor.Contract; +using BloomBooks.FreezeDoctor.Protocol; using NUnit.Framework; namespace BloomTests.FreezeDoctor { /// - /// Pins the shared-memory contract Bloom publishes for the Bloom Freeze Doctor - /// (https://github.com/BloomBooks/bloom-freeze-doctor). + /// Bloom's side of the protocol it shares with the Bloom Freeze Doctor + /// (https://github.com/BloomBooks/bloom-freeze-doctor): the layout it expects, and the health it + /// publishes through it. /// - /// **What this fixture is for changed when the contract became a package.** It used to guard against - /// two hand-maintained copies of the same file drifting apart. There is only one copy now — the - /// `BloomFreezeDoctor.Contract` package — so drift in that sense is no longer possible. + /// **What this fixture is for changed when the protocol became a package.** It used to guard against + /// two hand-maintained copies of the same file drifting apart. There is only one definition now — the + /// `BloomBooks.FreezeDoctor.Protocol` package — so drift in that sense is no longer possible. /// /// It still earns its place, for a different reason: it pins the layout Bloom *expects* against the /// layout the referenced package version actually has. A package upgrade that changed an offset or @@ -23,7 +24,7 @@ namespace BloomTests.FreezeDoctor /// rather than the numbers here being updated to match. /// [TestFixture] - public class DoctorChannelContractTests + public class FreezeDoctorProtocolTests { /// A process id no real Bloom will have, so a test run cannot collide with a live channel. private const int TestProcessId = 999_002; From e97a5c895bb7cb54c69b028584f6051a5624cdd7 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Fri, 21 Aug 2026 12:28:15 -0500 Subject: [PATCH 3/4] Move up to protocol 0.2.0-alpha, and pin the layout by value (BL-16719) The shared page now records PayloadBytes - how far the writer actually wrote - so that a Doctor newer than the Bloom it is reading can tell a field this Bloom never wrote from a real zero. See BloomBooks/bloom-freeze-doctor commit aaec915 for the reasoning; Bloom's side of it is just the reference bump, because Bloom keeps writing the same fields either way. The version had to move even though nothing is published: NuGet caches by id AND version, so re-packing 0.1.0-alpha with a changed layout would let a machine that had already restored it keep the old assembly, with nothing looking wrong anywhere. The tests are the substantive part. Bloom's pinned test asserted SchemaVersion, Size, ActivityMaxBytes and the section name - but not a single field offset, so a package upgrade that MOVED a field would have sailed through: Bloom would compile, run, and publish its health to offsets the Doctor no longer reads, producing reports that look plausible and are wrong. That was the whole failure mode the fixture existed to prevent, and it did not cover it. It does now, working from the layout's own published Fields table: - LayoutIsWhatBloomWasBuiltAgainst pins every field's name, offset and size by value, so a moved field fails Bloom's build. - AddingAFieldToTheProtocolDoesNotBreakBloom pins the invariant rather than the numbers: PayloadBytes must be exactly the end of the last field, must be at least the generation-1 baseline, and the baseline itself must stay 328. This is what lets the test above be strict without being a nuisance - the protocol is allowed to grow, and growth costs one line in the pinned list and changes nothing else on Bloom's side. - WhatBloomPublishesSaysHowMuchOfItIsReal checks the extent actually reaches the shared page, rather than trusting the constant to be right. Note the first test's comment says explicitly NOT to update the numbers to make it pass. A moved field means every already-installed Doctor is reading Bloom's page wrongly, so the numbers are the symptom. FreezeDoctor tests: 11 passed. Full suite: 3155 passed, 13 skipped, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomExe/BloomExe.csproj | 2 +- .../FreezeDoctor/FreezeDoctorProtocolTests.cs | 90 ++++++++++++++++++- 2 files changed, 87 insertions(+), 5 deletions(-) diff --git a/src/BloomExe/BloomExe.csproj b/src/BloomExe/BloomExe.csproj index 21f81ca931d3..bb492424eb09 100644 --- a/src/BloomExe/BloomExe.csproj +++ b/src/BloomExe/BloomExe.csproj @@ -241,7 +241,7 @@ One package means one definition of the wire format instead of two. Source: https://github.com/BloomBooks/bloom-freeze-doctor --> - + diff --git a/src/BloomTests/FreezeDoctor/FreezeDoctorProtocolTests.cs b/src/BloomTests/FreezeDoctor/FreezeDoctorProtocolTests.cs index 47a30483e16e..92f83f1431c3 100644 --- a/src/BloomTests/FreezeDoctor/FreezeDoctorProtocolTests.cs +++ b/src/BloomTests/FreezeDoctor/FreezeDoctorProtocolTests.cs @@ -1,4 +1,5 @@ using System; +using System.Linq; using Bloom.FreezeDoctor; using BloomBooks.FreezeDoctor.Protocol; using NUnit.Framework; @@ -30,11 +31,12 @@ public class FreezeDoctorProtocolTests private const int TestProcessId = 999_002; [Test] - public void LayoutMatchesTheDoctorsCopy() + public void LayoutIsWhatBloomWasBuiltAgainst() { - // If you are here because this failed: the layout changed. Update the copy of - // DoctorChannel.cs in BloomBooks/bloom-freeze-doctor, bump SchemaVersion in both, and update - // the pinned numbers in both repositories' tests. + // If you are here because this failed, a package upgrade changed the layout. Do NOT just update + // these numbers to match: check what moved and why. Adding a field should never reach this test + // (see the next one); anything that MOVES a field is a SchemaVersion bump, and Bloom's side of + // the protocol needs looking at before the numbers here are touched. Assert.That(DoctorChannelLayout.SchemaVersion, Is.EqualTo(1), "schema version"); Assert.That(DoctorChannelLayout.Size, Is.EqualTo(4096), "page size"); Assert.That( @@ -47,6 +49,86 @@ public void LayoutMatchesTheDoctorsCopy() Is.EqualTo(@"Local\BloomFreezeDoctor.v1.1234"), "the name must stay in the Local namespace and carry both version and pid" ); + + // Every field, by value, from the layout's own published description of itself. This is what + // turns "the package quietly moved a field" from a silent wrong-offset bug into a failed build. + var expected = new[] + { + "SchemaVersion@0+4", + "PayloadBytes@4+4", + "WriteSequence@8+8", + "ProcessId@16+4", + "ShutdownPhase@20+4", + "UiTicks@24+8", + "UiTimestamp@32+8", + "WatchdogTicks@40+8", + "WatchdogTimestamp@48+8", + "Flags@56+4", + "ServerBusy@60+4", + "ServerBlocked@64+4", + "Reserved@68+4", + "Activity@72+256", + }; + Assert.That( + DoctorChannelLayout.Fields.Select(f => $"{f.Name}@{f.Offset}+{f.Size}").ToArray(), + Is.EqualTo(expected), + "the field layout Bloom publishes to" + ); + } + + [Test] + public void AddingAFieldToTheProtocolDoesNotBreakBloom() + { + // The counterpart to the test above, and the reason it can be strict without being a nuisance. + // + // The protocol is allowed to GROW without a version bump: new fields are appended and + // PayloadBytes grows to match. When that happens the pinned list above gains an entry, but + // nothing Bloom does changes — Bloom keeps writing the fields it knows about, at the offsets it + // knows, and an older Doctor keeps reading them. + // + // So what is pinned here is not a number that may not change; it is the *invariant* that makes + // growth safe. If this fails, the layout has been rearranged rather than extended, and every + // Doctor already installed is reading Bloom's page wrongly. + var end = DoctorChannelLayout.Fields.Max(f => f.Offset + f.Size); + + Assert.That( + DoctorChannelLayout.PayloadBytes, + Is.EqualTo(end), + "PayloadBytes must be one past the last field, or a new field is outside what Bloom claims to have written" + ); + Assert.That( + DoctorChannelLayout.PayloadBytes, + Is.GreaterThanOrEqualTo(DoctorChannelLayout.BaselinePayloadBytes), + "the layout may only grow past the generation-1 baseline" + ); + Assert.That( + DoctorChannelLayout.BaselinePayloadBytes, + Is.EqualTo(328), + "the baseline is frozen for the life of schema version 1" + ); + } + + [Test] + public void WhatBloomPublishesSaysHowMuchOfItIsReal() + { + // A Doctor newer than this Bloom needs to be able to tell a field Bloom never wrote from a real + // zero. That only works if Bloom actually records its extent, so it is worth asserting that it + // reaches the page rather than trusting the constant. + using (var writer = new DoctorChannelWriter(TestProcessId)) + { + Assert.That(writer.IsOpen, Is.True, "setup: the channel should have been created"); + + Assert.That( + DoctorChannelReader.TryRead(TestProcessId, out var snapshot), + Is.True, + "setup: the channel should be readable" + ); + Assert.That( + snapshot.PayloadBytes, + Is.EqualTo(DoctorChannelLayout.PayloadBytes), + "Bloom must record how far it wrote, or a newer Doctor cannot tell absent from zero" + ); + } } [Test] From 4eb7463e74b9bee9943195107e2647dc095c8060 Mon Sep 17 00:00:00 2001 From: John Thomson Date: Fri, 21 Aug 2026 13:53:30 -0500 Subject: [PATCH 4/4] Detect a debugger attached to a running Bloom, and tidy two things (BL-16719) Moves to protocol 0.3.1-alpha and carries Bloom's half of two rounds of work. DEBUGGER DETECTION A debugger can be attached to an already-running Bloom, and used to stop it. Both leave exactly the evidence of a fault: one held at a breakpoint leaves a long hole in the UI heartbeat, which is what a freeze looks like, and terminating from a debugger is a TerminateProcess, so no clean exit is recorded and it looks like an unreported crash. Bloom read Debugger.IsAttached once at startup and never again, so neither case was detected at all. It is now polled every second by the watchdog thread - never on the UI thread - and published through a call that REMEMBERS, so a debugger that has come and gone is still visible to the Doctor afterwards. The startup read stays, so the value is right before the first tick. The check is `Debugger.IsAttached || IsDebuggerPresent()`. The native half matters: IsAttached only sees a MANAGED debugger, so WinDbg could attach to a running Bloom and stop it while IsAttached stayed false throughout - precisely the case that would otherwise be filed as a crash. Both are cheap enough to poll once a second; IsAttached reads a runtime flag and IsDebuggerPresent reads one byte of our own PEB. The P/Invoke is wrapped in a catch, because a diagnostic must never break the watchdog - which means a wrong DllImport signature would silently report "no debugger" for ever on every machine, and the sticky flag would never be set at all. So there is a test that calls it from outside that catch. That is the one thing here that could have degraded invisibly. Two limits, documented rather than papered over: a NON-INVASIVE attach (`windbg -pv`) sets neither flag and cannot be detected, and an attach that begins and ends inside one second is missed - though one that actually breaks or kills lasts far longer. TWO TIDIES DoctorLauncher looked in the parent directory as well as `current\`, in case a future Velopack went back to putting the launcher there. That is speculation about a direction Velopack seems unlikely to reverse, and the cost of being wrong is small and obvious: somebody on a newer Doctor launches it by hand. A check that exists for a hypothetical is one nobody can ever prove is still needed, so it is gone, with the reasoning recorded rather than left to be re-derived. UiHeartbeatInterval (500ms) and WatchdogInterval (1s) each said what they were for but not why they differ, which is the question a reader actually has: - The UI heartbeat is not a liveness check, it is the MEASUREMENT the tool rests on. Its staleness IS the freeze signal, so its interval sets that signal's resolution and the floor under how tight the Doctor's threshold can safely be - 5 seconds today, ten of these intervals. It is also the fragile one, since WM_TIMER is the lowest-priority message there is and a busy-but-live UI can starve it, so ticking twice as often means a moment's starvation must swallow two ticks rather than one before it looks like a freeze. - The watchdog is only a reference point, and nothing is measured against its resolution - it needs to be reliably alive, not finely sampled. It does real work per tick, and doubles as the latency of the Doctor's quit request, which that thread waits on rather than sleeping blindly. Full suite: 3157 passed, 13 skipped, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomExe/BloomExe.csproj | 2 +- src/BloomExe/FreezeDoctor/DoctorLauncher.cs | 12 ++-- .../FreezeDoctor/FreezeDoctorSupport.cs | 62 ++++++++++++++++- .../FreezeDoctor/FreezeDoctorProtocolTests.cs | 66 ++++++++++++++++++- 4 files changed, 133 insertions(+), 9 deletions(-) diff --git a/src/BloomExe/BloomExe.csproj b/src/BloomExe/BloomExe.csproj index bb492424eb09..554e79323288 100644 --- a/src/BloomExe/BloomExe.csproj +++ b/src/BloomExe/BloomExe.csproj @@ -241,7 +241,7 @@ One package means one definition of the wire format instead of two. Source: https://github.com/BloomBooks/bloom-freeze-doctor --> - + diff --git a/src/BloomExe/FreezeDoctor/DoctorLauncher.cs b/src/BloomExe/FreezeDoctor/DoctorLauncher.cs index 840a54f3c19f..0ae8398112ae 100644 --- a/src/BloomExe/FreezeDoctor/DoctorLauncher.cs +++ b/src/BloomExe/FreezeDoctor/DoctorLauncher.cs @@ -83,8 +83,14 @@ public static void LaunchIfInstalled() /// /// Looks for an installed Doctor. Velopack installs per-user into - /// %LOCALAPPDATA%\BloomFreezeDoctor\current\, and we accept a few nearby shapes so that a - /// developer running one from a build tree can be found too. + /// %LOCALAPPDATA%\BloomFreezeDoctor\current\, plus an environment variable so a developer + /// can point at a build tree. + /// + /// Only that one installed shape is looked for, deliberately. An earlier version also checked the + /// parent directory, in case a future Velopack went back to putting the launcher there — but that + /// is speculation about a direction Velopack seems unlikely to reverse, and the cost of being wrong + /// is small and obvious: somebody on a newer Doctor launches it by hand. A directory check that + /// exists for a hypothetical is a directory check nobody can ever prove is still needed. /// private static string FindInstalledDoctor() { @@ -95,8 +101,6 @@ private static string FindInstalledDoctor() { // The installed layout. Path.Combine(localAppData, "BloomFreezeDoctor", "current", ExecutableName), - // Some Velopack versions put the launcher one level up. - Path.Combine(localAppData, "BloomFreezeDoctor", ExecutableName), // An explicit override, for testing against a build tree. Environment.GetEnvironmentVariable("BLOOM_FREEZE_DOCTOR_PATH"), }; diff --git a/src/BloomExe/FreezeDoctor/FreezeDoctorSupport.cs b/src/BloomExe/FreezeDoctor/FreezeDoctorSupport.cs index abe5701ad4bf..5c253a892560 100644 --- a/src/BloomExe/FreezeDoctor/FreezeDoctorSupport.cs +++ b/src/BloomExe/FreezeDoctor/FreezeDoctorSupport.cs @@ -37,6 +37,14 @@ public static class FreezeDoctorSupport /// How often the UI thread reports in. Frequent enough that a freeze is obvious within the /// Doctor's shortest threshold, cheap enough to be invisible: this is one `WM_TIMER` and a /// handful of writes to a page of memory already resident. + /// + /// **Twice as often as the watchdog below, and the difference is deliberate.** This heartbeat is + /// not a liveness check, it is the *measurement* the whole tool rests on: its staleness is the + /// freeze signal, so its interval sets the resolution of that signal and the floor under how tight + /// the Doctor's threshold can safely be (5 seconds today, ten of these intervals). It is also the + /// fragile one — `WM_TIMER` is the lowest-priority message there is, and a busy-but-live UI really + /// can starve it — so ticking twice as often means a moment's starvation has to swallow two ticks + /// rather than one before it starts to look like a freeze. /// private static readonly TimeSpan UiHeartbeatInterval = TimeSpan.FromMilliseconds(500); @@ -44,6 +52,13 @@ public static class FreezeDoctorSupport /// How often the background thread reports in. Its purpose is comparison: if the UI heartbeat is /// stale and this one is fresh, the UI thread is blocked; if both are stale, the whole process is /// wedged — a garbage collection that will not finish, or a suspended process. + /// + /// Slower than the UI heartbeat because it is only a reference point, and nothing is measured + /// against *its* resolution — it needs to be reliably alive, not finely sampled. It also does real + /// work on each tick (publishes what Bloom is doing, polls for a debugger, and every tenth time + /// rewrites the session file), so ticking it faster would cost more for no gain. And it doubles as + /// the latency of the Doctor's quit request, which this thread waits on rather than sleeping + /// blindly: one second is how long a stuck Bloom takes to notice it has been asked to leave. /// private static readonly TimeSpan WatchdogInterval = TimeSpan.FromSeconds(1); @@ -137,8 +152,11 @@ public static void Start() // ComposeCurrentActivity retires this again once Bloom has handled a request. SetActivity(StartupActivity); // Authoritative, and worth more than the Doctor's own guess: it is why a developer - // stopping their debugger never produces a report. - _channel.SetDebuggerAttached(Debugger.IsAttached); + // stopping their debugger never produces a report. Published here so it is right from the + // start, and refreshed every second by the watchdog thread — a debugger can be attached to + // an already-running Bloom, and used to stop it, so reading this once would have missed + // exactly the cases that produce a bogus report. + _channel.SetDebuggerAttached(IsDebuggerAttached()); _uiHeartbeat = new System.Windows.Forms.Timer { @@ -430,6 +448,45 @@ private static string SafeCollectionName() /// are not the ones we care about. It does also refresh the session file, which is cheap and keeps /// that work off the UI thread. /// + /// + /// Reads the PEB's BeingDebugged flag for this process. Wanted alongside + /// because that one only sees a MANAGED debugger: a native one + /// (WinDbg without SOS, say) can attach to a running Bloom and stop it while IsAttached stays false + /// the whole time, which is precisely the case that would otherwise be reported as a crash. + /// + [System.Runtime.InteropServices.DllImport("kernel32.dll")] + [return: System.Runtime.InteropServices.MarshalAs( + System.Runtime.InteropServices.UnmanagedType.Bool + )] + internal static extern bool IsDebuggerPresent(); + + /// + /// Whether anything is debugging us, managed or native. + /// + /// Cheap enough to poll every second, which is what the watchdog does: IsAttached reads a runtime + /// flag, and IsDebuggerPresent reads a single byte out of our own process's PEB. Neither is a + /// blocking call and neither touches Bloom's UI thread. + /// + /// What it cannot see, for the record: a NON-INVASIVE attach (`windbg -pv`) sets neither flag, and a + /// debugger that attaches and detaches entirely between two polls is missed — though one that + /// actually breaks or kills lasts far longer than a second. + /// + internal static bool IsDebuggerAttached() + { + if (Debugger.IsAttached) + return true; + try + { + return IsDebuggerPresent(); + } + catch (Exception) + { + // A P/Invoke that cannot be resolved must not be able to break the watchdog; the managed + // answer above is still worth having. + return false; + } + } + private static void WatchdogLoop() { var sinceSessionRefresh = TimeSpan.Zero; @@ -456,6 +513,7 @@ private static void WatchdogLoop() { _channel?.RecordWatchdogTick(); PublishWhatBloomIsDoing(); + _channel?.SetDebuggerAttached(IsDebuggerAttached()); sinceSessionRefresh += WatchdogInterval; if (sinceSessionRefresh >= SessionRefreshInterval) { diff --git a/src/BloomTests/FreezeDoctor/FreezeDoctorProtocolTests.cs b/src/BloomTests/FreezeDoctor/FreezeDoctorProtocolTests.cs index 92f83f1431c3..14c027d8492b 100644 --- a/src/BloomTests/FreezeDoctor/FreezeDoctorProtocolTests.cs +++ b/src/BloomTests/FreezeDoctor/FreezeDoctorProtocolTests.cs @@ -1,4 +1,5 @@ using System; +using System.Diagnostics; using System.Linq; using Bloom.FreezeDoctor; using BloomBooks.FreezeDoctor.Protocol; @@ -68,6 +69,7 @@ public void LayoutIsWhatBloomWasBuiltAgainst() "ServerBlocked@64+4", "Reserved@68+4", "Activity@72+256", + "DebuggerLastDetached@328+8", }; Assert.That( DoctorChannelLayout.Fields.Select(f => $"{f.Name}@{f.Offset}+{f.Size}").ToArray(), @@ -101,13 +103,73 @@ public void AddingAFieldToTheProtocolDoesNotBreakBloom() Is.GreaterThanOrEqualTo(DoctorChannelLayout.BaselinePayloadBytes), "the layout may only grow past the generation-1 baseline" ); + // Equal to PayloadBytes for now, because generation 1 is unreleased and there is no older Bloom + // writing less. Once a Bloom ships writing this page, this number freezes and appending a field + // grows only PayloadBytes. Assert.That( DoctorChannelLayout.BaselinePayloadBytes, - Is.EqualTo(328), - "the baseline is frozen for the life of schema version 1" + Is.EqualTo(336), + "the generation-1 floor" ); } + [Test] + public void TheNativeDebuggerCheckActuallyResolves() + { + // Bloom's debugger check swallows exceptions, because a diagnostic must never be able to break + // the watchdog thread. That means a wrong DllImport signature would not fail loudly — it would + // just report "no debugger" for ever, and the sticky flag would never be set on any machine. + // Calling it here, outside that catch, is what turns a broken P/Invoke into a failing test. + Assert.DoesNotThrow( + () => FreezeDoctorSupport.IsDebuggerPresent(), + "the kernel32 IsDebuggerPresent import should resolve" + ); + + // Whatever the environment, the combined answer must agree with the managed one when that says + // a debugger is attached — this is the direction that matters, since it is what suppresses + // reports while a developer is stepping through Bloom. + if (Debugger.IsAttached) + Assert.That( + FreezeDoctorSupport.IsDebuggerAttached(), + Is.True, + "a managed debugger must always count as attached" + ); + else + Assert.DoesNotThrow(() => FreezeDoctorSupport.IsDebuggerAttached()); + } + + [Test] + public void ADebuggerThatHasComeAndGoneIsStillVisibleToTheDoctor() + { + // Bloom's end of the sticky flag. The Doctor's repo tests the mechanism; what is worth checking + // here is that Bloom is publishing through the call that remembers, so that a debugger which + // attached and left does not leave a heartbeat gap looking like a genuine freeze. + using (var writer = new DoctorChannelWriter(TestProcessId)) + { + Assert.That(writer.IsOpen, Is.True, "setup: the channel should have been created"); + + writer.SetDebuggerAttached(true); + writer.SetDebuggerAttached(false); + + Assert.That(DoctorChannelReader.TryRead(TestProcessId, out var snapshot), Is.True); + Assert.That( + snapshot.DebuggerAttached, + Is.False, + "setup: it should have gone again" + ); + Assert.That( + snapshot.DebuggerEverAttached, + Is.True, + "but the Doctor must still be able to tell that one was here" + ); + Assert.That( + snapshot.DebuggerLastDetachedAge, + Is.LessThan(TimeSpan.FromMinutes(1)), + "and roughly when it left, so an unrelated freeze later is still reportable" + ); + } + } + [Test] public void WhatBloomPublishesSaysHowMuchOfItIsReal() {