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..e908888e5425 100644 --- a/.gitignore +++ b/.gitignore @@ -199,3 +199,9 @@ pr-comments*.md # run build/ci/watch-for-hung-bloom.ps1 locally). Never meant to be committed. bloom-diagnostics/ bloom-watchdog/ + +# 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 new file mode 100644 index 000000000000..f0ce80fae330 --- /dev/null +++ b/NuGet.Config @@ -0,0 +1,26 @@ + + + + + + + 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-freeze-doctor-protocol.ps1 b/build/pack-freeze-doctor-protocol.ps1 new file mode 100644 index 000000000000..ce336c730304 --- /dev/null +++ b/build/pack-freeze-doctor-protocol.ps1 @@ -0,0 +1,46 @@ +# 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 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" + +$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\BloomBooks.FreezeDoctor.Protocol\BloomBooks.FreezeDoctor.Protocol.csproj" + +if (-not (Test-Path $project)) { + 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 protocol 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/bloombooks.freezedoctor.protocol" diff --git a/localpackages/.gitkeep b/localpackages/.gitkeep new file mode 100644 index 000000000000..da6c6507ac5e --- /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-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 8d2f19c1f7c9..554e79323288 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/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/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..5c253a892560 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 BloomBooks.FreezeDoctor.Protocol; using SIL.Reporting; namespace Bloom.FreezeDoctor @@ -34,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); @@ -41,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); @@ -134,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 { @@ -427,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; @@ -453,6 +513,7 @@ private static void WatchdogLoop() { _channel?.RecordWatchdogTick(); PublishWhatBloomIsDoing(); + _channel?.SetDebuggerAttached(IsDebuggerAttached()); sinceSessionRefresh += WatchdogInterval; if (sinceSessionRefresh >= SessionRefreshInterval) { diff --git a/src/BloomTests/FreezeDoctor/DoctorChannelContractTests.cs b/src/BloomTests/FreezeDoctor/FreezeDoctorProtocolTests.cs similarity index 55% rename from src/BloomTests/FreezeDoctor/DoctorChannelContractTests.cs rename to src/BloomTests/FreezeDoctor/FreezeDoctorProtocolTests.cs index a5d230a55db2..14c027d8492b 100644 --- a/src/BloomTests/FreezeDoctor/DoctorChannelContractTests.cs +++ b/src/BloomTests/FreezeDoctor/FreezeDoctorProtocolTests.cs @@ -1,33 +1,43 @@ using System; +using System.Diagnostics; +using System.Linq; using Bloom.FreezeDoctor; +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. /// - /// **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 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 + /// 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 + 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; [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( @@ -40,6 +50,147 @@ 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", + "DebuggerLastDetached@328+8", + }; + 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" + ); + // 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(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() + { + // 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]