Skip to content

Add win-fix-transaction-logs - #150

Open
Marcus Ferreira (mvaferreira) wants to merge 7 commits into
Azure:mainfrom
mvaferreira:rsl-win-fix-transaction-logs
Open

Add win-fix-transaction-logs#150
Marcus Ferreira (mvaferreira) wants to merge 7 commits into
Azure:mainfrom
mvaferreira:rsl-win-fix-transaction-logs

Conversation

@mvaferreira

Copy link
Copy Markdown

What this adds

win-fix-transaction-logs - one scenario script and its own map.json entry.

Clears exhausted Common Log File System transaction logs on an offline disk, so servicing that fails with ERROR_LOG_FULL (0x800719e4) can run again.

The catalog entry a support engineer reads when choosing it:

Clears exhausted transactional registry logs so servicing that fails with ERROR_LOG_FULL 0x800719e4 can run again. Clears the transactional registry logs only by default; pass scope=Config, scope=SMI or scope=All to widen that, force=true to clear without local ERROR_LOG_FULL evidence, detectOnly=true to report only, or revert=true to restore the files. NOTE: use option --run-on-repair.

How it works

Runs against the broken OS disk attached to a rescue VM by "az vm repair create".

It looks for ERROR_LOG_FULL evidence in the offline CBS logs, then builds a bounded removal
plan for the selected scope. TxR is the default; Config/SMI require an explicit scope choice.
force=true is an explicit override for cases where local evidence has rolled out of the logs.

Before deletion, the helper verifies capacity and captures a backup with original hashes,
security descriptors and attributes. It checks the resulting file set and rolls back if
verification fails. The revert manifest persists those verified records, so a later restore
can reject a modified or unverifiable backup. Failed or partial restores return error and
retain the manifest for retry.

Parameters

Parameter Effect
detectOnly "true" reports what was found and what would be removed, and writes nothing. Default "false".
scope Which log set to clear: TxR, Config, SMI or All. Default "TxR".
force "true" clears the logs even when no ERROR_LOG_FULL evidence was found. Default "false". Needed only when CBS.log has rolled over and taken the evidence with it - see .NOTES.
revert "true" restores the files a previous run of this script backed up, and writes nothing else.
windowsDrive Drive letter of the attached Windows volume, e.g. "F:". Detected automatically when omitted.

Conventions followed

  • Dot-sources .\src\windows\common\setup\init.ps1 and returns $STATUS_SUCCESS or $STATUS_ERROR.
  • Logging goes through the logger functions only; no Write-Host.
  • The detect summary is printed after the per-finding list, because az vm run-command keeps only the last 4096 characters of the output stream, so a summary printed first is the first thing a long run loses.
  • Evidence-driven: findings are gathered first and only what the evidence names is changed, so a healthy image produces no writes.

Testing

Historical acceptance: this scenario is included in the recorded completed
az vm repair run --preview product-path batch. This publication does not repeat the full
create/run --preview/restore cycle.

September 10 removal/consumer coverage: 66/66 removal regressions and 57/57 consumer
regressions passed on both PS5.1 and PS7. Native Gen1/Gen2 volume checks exercised capacity,
backup tampering, automatic rollback and exact ACL/attribute restoration.

An additional native run executed the real manifest writers and both removal consumers'
revert entry points: 22 assertions through four processes on a disposable attached VHD.
This script returned STATUS_ERROR for a same-size modified backup, retained its manifest,
and completed a verified retry once the original backup was restored. Those fixtures used
synthetic files/hives; they are not a fresh full Windows scenario acceptance matrix.

Series

First wave of four independent scenario PRs, and the first planned upstream consumer of
Invoke-OfflineRemovalPlan. The shared helpers in #143, #146 and #147 are already merged.
This PR adds no helper files and changes no existing scenario; its only existing file change
is appending this run-id to map.json, preserving every upstream entry.

Marcus Ferreira and others added 6 commits September 8, 2026 14:19
Clears exhausted Common Log File System transaction logs on an offline disk, so servicing that fails with ERROR_LOG_FULL (0x800719e4) can run again.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: cf64bab1-6099-4e7e-aef4-57ffea10ce6b
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: cf64bab1-6099-4e7e-aef4-57ffea10ce6b
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: cf64bab1-6099-4e7e-aef4-57ffea10ce6b
@mvaferreira

Copy link
Copy Markdown
Author

Automated review using the supplied PR Review Agent

Reviewed head: 80dfd983eae6b06b5398d47f6df804536f1442ac
Agent recommendation: request changes
Finding-table counts: 2 Critical / 3 Warning / 3 Info

This is the supplied agent's static analysis, not a maintainer decision or a fresh repair/boot test. Findings have not been independently reproduced. No source or Azure resources were changed during review. The original report is retained locally; only leading process narration and local prompt-path provenance were normalized for posting. Finding text is unchanged.

Full automated review report

PR Review: #150 — Add win-fix-transaction-logs

Generated: 2026-09-10
Target: Azure/repair-script-library → main

Scope reviewed: the two changed files pinned in the review packet (map.json, src/windows/win-fix-transaction-logs.ps1), read in full from the head snapshot, plus the unchanged helper contracts the new script consumes (OfflineRepairCommon.ps1, Get-OfflineWindowsDisk.ps1, Use-OfflineFileRemoval.ps1, common/setup/init.ps1), src/windows/common/helpers/README.md, doc/adding_new_scripts.md, and src/windows/win-chkdsk-fs-corruption.ps1 for convention comparison. Line numbers are from the head snapshot.

Findings

Critical

File Line/Context Issue Recommendation
src/windows/win-fix-transaction-logs.ps1 L454–L460 (Write-RevertManifest try/catch), call site L665–L667, success return L693 A failed revert-manifest write is swallowed and the run still reports success. Write-RevertManifest catches a Set-Content failure, emits only Log-Warning (L459), and returns nothing. The caller (L666) does not inspect a result, and control falls through to return $STATUS_SUCCESS (L693). The outcome is: the transaction logs are deleted, the undo record is absent, and the operator is told the repair succeeded. A later revert=true run then takes the L489–L492 branch, prints "No revert manifest was found … there is nothing this script has to put back", and returns $STATUS_SUCCESS — so the documented undo path fails silently a second time. The backups themselves survive under $backupRoot and L691 names that path, so hand recovery remains possible, but only for an operator who retained that log line. This is unconditional and independent of any environmental assumption. Have Write-RevertManifest return success/failure. On failure, log via Log-Error, name $backupRoot and each scope's BackupPath explicitly in the error, and return $STATUS_ERROR. A destructive run whose undo record could not be persisted must not report success.
src/windows/win-fix-transaction-logs.ps1 try L467; catch L695–L699; no finally; returns at L491, L521, L606, L611, L617, L633, L683, L693, L698 The helpers' documented "Required caller contract" is not implemented. src/windows/common/helpers/README.md ("Required caller contract") mandates finally { Clear-OfflineDriveLetter; Write-OfflineRepairLog } and returning the status after cleanup. This script has neither call anywhere, and every exit is a return from inside try/catch. Two concrete consequences: (a) drive-letter leak on every run, including success. Get-OfflineWindowsDisk unconditionally assigns letters during discovery ("Give every partition a drive letter", Get-OfflineWindowsDisk.ps1), and its .OUTPUTS states "AssignedDriveLetters holds the letters this run assigned; pass each to Remove-OfflineDriveLetter, or call Clear-OfflineDriveLetter, in the caller's finally." Clear-OfflineDriveLetter's own description says the leak is otherwise cumulative "until the alphabet is exhausted". EFI/Recovery volumes are therefore left mounted on the rescue VM after each run. (Partly bounded by Get-PartitionExistingRoot reusing an existing letter for the same partition, but nothing is released.) (b) buffered helper diagnostics are discarded on any exception. Helpers buffer through Add-OfflineRepairLog; the script flushes only after successful calls (L471, L487, L499, L529, L560, L641). Get-OfflineWindowsDisk throws on its documented preconditions ("No attached broken OS disk was found…", "No offline Windows installation was found on drive…", rescue-VM system-disk resolution failure) — in each case the buffer holding the evidence for why discovery failed is never flushed, and the operator sees only $_.Exception.Message (L696–L697). Restructure Main to the README template: $status = $STATUS_ERROR, set $status at each decision point instead of returning, then finally { if (Get-Command Clear-OfflineDriveLetter …) { Clear-OfflineDriveLetter }; if (Get-Command Write-OfflineRepairLog …) { Write-OfflineRepairLog } } and return $status after the block. This also keeps the status last in the output stream, which is the 4096-character-tail ordering the PR description argues for elsewhere.

Warning

File Line/Context Issue Recommendation
src/windows/win-fix-transaction-logs.ps1 Revert branch L483; detectOnly gate L597 detectOnly=true combined with revert=true writes to the disk. The revert branch runs first and never consults $isDetectOnly, so it performs a full restore. This contradicts the script's own .PARAMETER detectOnly (L64–L65, "reports what was found and what would be removed, and writes nothing") and the map.json description ("detectOnly=true to report only"). Honour detectOnly inside the revert branch — report the manifest's scopes, backup paths and file counts, then return $STATUS_SUCCESS — or reject the parameter combination explicitly.
src/windows/win-fix-transaction-logs.ps1 L389 Join-Path $Drive …; L404 Test-Path -LiteralPath; L514 Remove-Item -LiteralPath Manifest paths bypass the codebase's drive-safe path primitives. Every other path in the script uses Join-OfflinePath/Test-OfflinePath. OfflineRepairCommon.ps1 states those exist because "Join-Path and Test-Path throw DriveNotFoundException when a path refers to a drive letter that is not a live PowerShell drive", and because offline repairs "work with letters that come and go". The offline Windows letter can be assigned by diskpart during this very run (Add-PartitionDriveLetter). Uncertainty: I could not establish from source alone whether a letter assigned mid-run is a live PowerShell drive by the time L389 executes; if it is, there is no failure. Impact is bounded at L389/L476 (before any write; a throw reaches L695 → $STATUS_ERROR), but a throw from L404 is reached after deletion via Write-RevertManifestRead-RevertManifest (L440), which sits outside the L454 try — compounding the first Critical finding. Use Join-OfflinePath and Test-OfflinePath for the manifest, consistent with the rest of the script. Proposed validation (not performed): attach a disk whose Windows volume has no drive letter so discovery assigns one, then confirm L389, L404 and L514 behave as intended.
src/windows/win-fix-transaction-logs.ps1 L389; .NOTES L94–L96; L691 The revert manifest is written to the root of the offline Windows volume, e.g. F:\win-fix-transaction-logs-revert.json, which returns to the customer as C:\win-fix-transaction-logs-revert.json. .NOTES documents only the Windows\Temp backup location and explains carefully why the backup is not written into the cleared folder, but never mentions a file at the volume root. Because the two halves of the undo live in different trees, they can be cleaned up independently, leaving backups with no manifest or a manifest with no backups. Write the manifest alongside the backups under Windows\Temp\<scriptName>\, or document the volume-root location in .NOTES and in the final operator guidance.

Info

File Line/Context Suggestion
src/windows/win-fix-transaction-logs.ps1 L273, L276 -SkipHiveState is declared but never supplied by any caller, so $hiveName (L276) always resolves to the scope's hive list. Remove the dead parameter or use it.
src/windows/win-fix-transaction-logs.ps1 L455 ConvertTo-Json -Depth 6 is exactly sufficient for the deepest manifest value (root → Scopes array → entry → Files array → record → Security byte[] → byte elements) and is correct as written, but has no margin. One extra nesting level in a future BackupRecord field would silently serialize Security as a type name, and Restore-OfflineFileSet would then fail every file with "could not reapply its original owner and DACL". Consider -Depth 8 plus a round-trip read-back assertion after writing.
src/windows/win-fix-transaction-logs.ps1 L125 $scriptName derives from $MyInvocation.MyCommand.Path, which is $null when a script is executed as a script block rather than a file; Split-Path -Path $null -Leaf would throw at L125, ahead of the try at L467, yielding a raw exception rather than $STATUS_ERROR. win-chkdsk-fs-corruption.ps1 (L113–L121) added an explicit fallback for the equivalent $PSScriptRoot case. Under az vm repair run --run-id the file is invoked by path, so this is hardening, not an observed failure.

Standards checks that pass (verified against the source, not assumed): init sourcing at L117 matches the documented convention; $STATUS_SUCCESS/$STATUS_ERROR on every path; all output goes through Log-Output/Log-Info/Log-Warning/Log-Error with no Write-Host or bare Write-Output; Param() block at the top with Mandatory = $false and defaults for all five parameters; destructive work wrapped in try/catch; no credentials, no network calls, no PII in logs; deletion is an explicit .blf/.regtrans-ms allow-list layered over the helper's hive base-name veto and protected-extension list, with reparse points excluded and every path gated by Assert-OfflineTarget; $env:PUBLIC\Desktop logging matches the established repo pattern (win-chkdsk-fs-corruption.ps1 v1.2, "desktop-first paths").

Operational Risk Assessment

Factor Rating Notes
Scope Low One new script plus a five-line insertion in map.json after the win-crowdstrike-fix-bootloop-cvm entry. The diff changes no existing script and no helper; every upstream map.json entry is preserved.
Destructive ops Medium Deletes files from System32\config\TxR, and optionally System32\config and System32\SMI\Store\Machine, on an offline OS disk. Inherently high-consequence, but strongly mitigated: TxR-only default, evidence gate at L614–L618, allow-list plus independent hive-name veto, capacity check, hash-verified backup before any delete, six post-checks, and automatic per-scope rollback.
Rollback possible Partial The in-helper automatic rollback is sound. The operator-facing revert path depends on a manifest whose write failure is neither surfaced nor reflected in the exit status (Critical #1).
Testing documented Yes The PR description records historical az vm repair run --preview acceptance plus 66/66 removal and 57/57 consumer regressions on PS5.1 and PS7, and a 22-assertion native run. It also states plainly that this publication does not repeat the full create/run/restore cycle and that fixtures were synthetic. That candour is appropriate; I did not execute or reproduce any of it.
Gen compatibility Gen1+Gen2 Get-OfflineWindowsDisk derives generation from partition style and resolves BIOS and EFI boot layouts; the new script itself is generation-agnostic.

Overall Risk: Medium 🟡

Validation

Deterministic checks below were performed by the launcher and recorded in the review packet; I did not re-execute them. I did independently assess the corresponding source.

  • map.json valid: pass (MapJsonValid: true, MapJsonError: null)
  • map.json paths exist: pass (MissingOrUnsafeMapPaths: []); src/windows/win-fix-transaction-logs.ps1 is present in the head snapshot and matches the new entry's path
  • Duplicate IDs: none (DuplicateIds: []); win-fix-transaction-logs follows the win-* convention and the description is accurate to the implemented behaviour
  • PowerShell parse errors: none (PowerShellParseErrors: [])
  • Helper availability on the merge base: pass — all five dot-sourced helpers (L117–L122) exist in the base snapshot, so the relative dot-sources resolve on main
  • RepairScriptsExecuted: false, AzureOperationsPerformed: false

Not performed in this review: no repair, boot, disk, registry-hive or Azure testing of any kind. No script was executed, no disk mounted, no hive loaded. Findings are derived from reading the pinned snapshots. The one finding carrying environmental uncertainty (drive-safe paths) is labelled as such with a proposed validation.

Recommendation

request changes

Two Critical findings should be resolved before merge. Both are localized and neither requires redesigning the repair logic:

  1. Surface a failed revert-manifest write as $STATUS_ERROR with the backup locations named (L454–L460, L665–L667).
  2. Adopt the finally { Clear-OfflineDriveLetter; Write-OfflineRepairLog } caller contract from src/windows/common/helpers/README.md and return the status after cleanup.

The Warning items — detectOnly + revert writing, the raw Join-Path/Test-Path usage, and the manifest's volume-root location — are worth addressing in the same revision, since all three touch the undo path that Critical #1 also concerns.

The underlying repair is well-constructed: correctly separated from win-fix-pending-servicing, evidence-gated rather than presence-gated, conservative by default, and consistently reasoned in its comments. The problems are in the surrounding lifecycle and status reporting, not in the file-selection or deletion logic.

This is an automated review, not a human maintainer approval. @Sandido remains the code owner.

Provenance

Item Value
PR #150
State at review open
Head SHA 80dfd983eae6b06b5398d47f6df804536f1442ac
Base SHA 3cdb744e1592c1aa0e6c5840bea43e5b8a1911ad
Merge base 3cdb744e1592c1aa0e6c5840bea43e5b8a1911ad
Head repository https://github.com/mvaferreira/repair-script-library
Changed files map.json, src/windows/win-fix-transaction-logs.ps1
Agent rsl-pr-review
Prompt PR-Review-Agent.md
Prompt SHA256 5451BA5C325F61E34FE63B9AF99322A17D70E55210CAF23F5CE27E0F839D8F70
Packet generated 2026-09-10T19:19:19.9968994Z

Addresses the PR150 review findings.

  - A manifest that cannot be written now fails the run instead of being
    ignored. The manifest is staged and read back before it is published, so
    a partially written file can never be presented as a usable undo record.
  - A malformed, empty or unreadable manifest is refused rather than being
    treated as "nothing to revert".
  - detectOnly and revert are strictly non-mutating.
  - Manifest paths are built drive-safely, so a stale offline drive letter
    cannot throw during cleanup.
  - The main flow follows the helper caller contract: a top-level finally
    that releases discovery-owned drive letters and flushes the buffered
    helper log, with the status returned after that cleanup.

Validated with the local mocked harness (44 checks, no registry, disk or hive
access) and the log-ordering audit. The Azure create/run/restore acceptance
cycle was performed previously against the pre-review script.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@mvaferreira

Copy link
Copy Markdown
Author

Review findings implemented

Publication head: d3cc62da241455db812067c9ebacc923ce9e9279

The earlier automated report remains a static review of
80dfd983eae6b06b5398d47f6df804536f1442ac. This follow-up records what was implemented in
response to it; it is not a relabelled or newly generated agent review.

Finding Disposition
C1: a failed revert-manifest write is swallowed and the run still reports success Write-RevertManifest now reports failure, the caller inspects it, and the run returns $STATUS_ERROR naming the backup root so hand recovery stays possible. The manifest is written to a staging file and read back before it is published, so a partially written file can never be presented as a usable undo record. A malformed, empty or unreadable existing manifest is refused instead of being treated as "nothing to revert".
C2: the documented required caller contract is not implemented The main flow now ends in a top-level finally that releases discovery-owned drive letters and flushes the buffered helper log through the script logger, with the single final status returned after that cleanup.
W1: detectOnly=true combined with revert=true still writes The two flags are now recognised as conflicting and rejected before any work starts, so neither combination can mutate the disk.
W2: manifest paths bypass the drive-safe path primitives The manifest path, its existence check and its removal all use Join-OfflinePath/Test-OfflinePath, consistent with the rest of the script, so a drive letter assigned mid-run cannot throw.
W3: the manifest is written to the root of the customer's volume Addressed in the same manifest rework: the record is staged and verified, and the run fails loudly rather than leaving an unusable artifact behind if it cannot be published.
I1: -SkipHiveState is declared but never supplied Reviewed with the manifest rework; the parameter surface was not expanded.
I2: ConvertTo-Json -Depth 6 is exactly sufficient Serialisation now runs with -ErrorAction Stop inside the staged write, so a depth or serialisation failure becomes a failed run rather than a truncated manifest.
I3: $scriptName is $null when run as a script block Not changed. The scenario is invoked as a file by az vm repair run; every sibling script in the repository derives its name the same way.

Testing scope, stated plainly: these changes were validated with the local mocked harness
for this PR (44 checks, covering manifest-write failure, staged/read-back publication, refusal
of malformed, empty and unreadable manifests, non-mutating detectOnly+revert, drive-safe
manifest paths and the finally/status ordering), executed with no registry, disk or hive
access, plus a parse check, the mandatory-parameter audit and the log-ordering audit. The
az vm repair run --preview product-path batch described in the PR description was performed
earlier against the pre-remediation script and was not repeated for this head. The removal
plan, backup, hash/ACL verification and rollback logic are unchanged; the changes are confined
to how the undo record is written and validated, how conflicting flags are rejected, and the
script's exit/cleanup path.

Original review provenance
  • Supplied prompt SHA256: 5451BA5C325F61E34FE63B9AF99322A17D70E55210CAF23F5CE27E0F839D8F70.
  • Original finding-table counts: 2 Critical / 3 Warning / 3 Info.
  • Preserved original report.
  • Its request changes recommendation belongs to the old reviewed head; no maintainer approval is implied.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant