Sweep every exported file on a failed export, and record the guards' ancestor blind spot (#1973)

[erikdarlingdata]

Closes #1973. Both items are the two review findings dispositioned on #1968 but deferred at merge time; all changes are in Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs.

Item 1 - a failed export sweeps all three files and names what survived

The catch around the three WriteExportFileAsync calls deleted only darling.json. Since each write deletes the previous export's copy before recreating it, a failure on server.crt or README.txt left a mix of new and stale files with no config beside them, and the error named none of it - which reads to an operator as "nothing was written" when something was.

The catch now sweeps all three through a new TryRemovePartialExport helper. Every file still on disk is named, either as removed or as left behind with the OS reason. The darling.json line keeps its live-password framing verbatim; the cert and README are phrased as cleanup, because neither carries a secret and neither is usable without the config. Each removal is independent, so a delete that itself fails masks neither the original error nor the other two removals.

One detail worth calling out: the existence probe is File.GetAttributes in a try, not File.Exists - the idiom TryCheckExportTarget already uses ten lines away. File.Exists answers false for a dangling symlink, which would leave a planted link unmentioned by the one sweep whose entire job is to account for what is on disk.

The error shape, verbatim from a live run of the failure path

Captured from the rebuilt binary (ExportViewerConfigAsync with the cert write forced to fail), stderr as written:

WARNING: C:\...\handoff\darling.json will contain a LIVE database password (the 'viewer' role) in cleartext, because that is what the viewer connects with. Copy the folder over a channel you trust, and keep the file ACL'd to the operator account on the viewer machine.
Could not write the viewer config to C:\...\handoff: The process cannot access the file 'C:\...\handoff\server.crt' because it is being used by another process.
Removed C:\...\handoff\darling.json (it held a live password - the whole of one if the write that failed was a later file).
WARNING: C:\...\handoff\server.crt was left behind (The process cannot access the file 'C:\...\handoff\server.crt' because it is being used by another process.) - it is a leftover of the failed export, not a secret. Delete it yourself.

(Paths elided to C:\... for width; the run used a temp directory. The em dashes in the "held a live password" and "not a secret" clauses are real em dashes in the source, matching this file's existing operator-facing voice. README.txt is absent from the output because the run never reached its write - there is nothing on disk to name, which is the correct silence.)

Item 2 - the directory guards' ancestor blind spot, recorded

Comment only, on both the lexical self-destruct compare and the leaf reparse-point check. Path.GetFullPath normalizes text without resolving links, and DirectoryInfo.Attributes describes only the last path component, so a junction on an ancestor of the destination slips past both.

Deliberately not chased with a chain-resolving walk, and the comment records why: the destructive case the reviewer named - the export resolving back into the service's config directory and destroying the service's own darling.json - is already refused one layer down. TryCheckExportTarget runs per file through the OS's real path resolution (File.GetAttributes and File.ReadAllText follow the whole junction chain to the actual file), and the service's own darling.json carries no export marker, so such a run stops with "not written by --export-viewer-config" before anything is deleted. What survives the gap is only "the files land in a directory the junction-planter chose" - and planting a junction in the ancestor chain of the operator's destination already means controlling that destination's fate.

Test

ExportViewerConfigAsync_FailsPartWayThrough_SweepsAllThreeAndNamesWhatSurvived in Darling/Darling.Tests/DarlingExportViewerConfigTests.cs.

The failure is forced where it actually bites: after darling.json has been written and hardened, on the server.crt write. A pre-existing (marker-carrying) cert is held open with FileShare.Read - readable, so the pre-flight identity check passes and the run reaches the writes at all, but not deletable, so WriteExportFileAsync's delete-then-CreateNew fails with the same IOException a live antivirus or an open editor produces. No stub, no seam injection; this is the production write path.

Asserts exit 1, that darling.json is gone and named as the secret it was, that the surviving server.crt is named as left behind (and still holds its stale content), that README.txt was never reached, and - the claim the whole sweep rests on - that every file remaining in the folder appears by name in stderr, enumerated from Directory.GetFiles. Also that the password reaches neither stream on the failure path.

Watched red

With the source change reverted and the test project rebuilt (not --no-build against a stale DLL), the new test fails on exactly the old behavior:

[FAIL] ExportViewerConfigAsync_FailsPartWayThrough_SweepsAllThreeAndNamesWhatSurvived
Assert.Contains() Failure: Sub-string not found
DarlingExportViewerConfigTests.cs:line 845

Line 845 is Assert.Contains($"{exportedCert} was left behind", stderr, ...) - the old code removed darling.json and left the cert behind unnamed. Source change restored, retested green.

Gates

• Build: Build succeeded. 0 Warning(s) 0 Error(s) (checked the warnings line, not just the exit code).
• Test project rebuilt before every run.
• DarlingExportViewerConfigTests: 39 passed, 1 skipped, 0 failed. The skip is the pre-existing symlink test, which needs Developer Mode or elevation.
• Full ungated Darling.Tests: 3898 passed, 0 failed, 223 skipped (the live-Postgres gated classes).
• CHANGELOG entry under [Unreleased] > Fixed with the bottom link-ref.

Windows-only conventions followed: Assert.SkipUnless(OperatingSystem.IsWindows(), "DPAPI requires Windows."), temp subdirectory with finally cleanup, StringWriter capture, exit-code asserts.

🤖 Generated with Claude Code

[claude]

Minor/non-blocking: there's a small TOCTOU gap here — File.GetAttributes (existence probe) and the later File.Delete are two separate filesystem calls, so a file that vanishes between them (e.g. another process races to delete it first) would hit the generic catch (Exception cleanupFailure) in the delete block rather than being treated as "already gone." Worst case is a slightly misleading "left behind" warning for a file that's actually not there — cosmetic only, given this is a single-process CLI cleanup path and File.Delete is itself a no-op-safe call for already-missing files in the common case. Not asking for a change, just flagging since the surrounding code is otherwise very precise about exactly what each exception type means.

[claude]

Review

Scope: entirely within Darling/PerformanceMonitor.Darling.Service/DarlingCliCommands.cs (+ its test file and CHANGELOG). No T-SQL touched, and no Lite-side changes — checked, and Lite has no export-viewer-config/managed-Postgres viewer feature at all, so this isn't a parity-drift case; the two apps just don't share this surface.

Item 1 — sweep all three files on a failed export

Traced the full path: ExportViewerConfigAsync's catch block now calls the new TryRemovePartialExport for configFile, certificateFile, and readmeFile in write order, each independently try/caught so one failure can't mask another or the original error. Verified against the code (not just the PR description):

• TryRemovePartialExport correctly uses File.GetAttributes rather than File.Exists for the existence probe. This mirrors TryCheckExportTarget's existing idiom, and the claim that File.Exists returns false for a dangling symlink while GetAttributes still reports the reparse point is empirically pinned by the pre-existing ExportViewerConfigAsync_SymlinkAtTheCertificatePath_RefusesWithoutWritingThrough test (line 762 asserts exactly this precondition on Windows). So the "why GetAttributes not Exists" reasoning in the new method's comment isn't just asserted, it's already load-bearing elsewhere in this file.
• Message templates check out: the darling.json line keeps the "live password" framing, cert/README get the "cleanup, not a secret" framing, matching the transcript in the PR description.
• No secret leakage: confirmed the password never reaches either stream on the failure path (the new test asserts DoesNotContain("viewer-secret-pw", ...)), and the only content written to error is filenames/exception messages.
• The new DarlingExportViewerConfigTests test forces the failure at the right seam (a real FileShare.Read lock on server.crt, not a stub), which exercises the actual delete-then-CreateNew write path in WriteExportFileAsync rather than a mocked failure. Good production-path coverage, and the assertion that walks Directory.GetFiles(destination) and requires every survivor be named by string-contains in stderr is a strong test of the actual claim ("nothing left unaccounted for"), not just a fixed set of expected lines.

Left one non-blocking inline note on a minor TOCTOU gap between the GetAttributes check and the Delete call in TryRemovePartialExport (two syscalls, not one) — cosmetic given this is a single-process CLI cleanup path, not requesting a change.

Item 2 — ancestor-junction blind spot, comment-only

Confirmed this is genuinely comment-only (no behavior change) on both the lexical Path.GetFullPath self-destruct guard and the leaf DirectoryInfo.Attributes.HasFlag(ReparsePoint) check. The backstop claim holds up: TryCheckExportTarget (unchanged) reads via File.GetAttributes/File.ReadAllText, which do follow the full reparse chain to the real target, and the service's own darling.json won't carry the ViewerConfigMarker, so the destructive overwrite case is refused independent of the ancestor-junction gap. Reasonable to leave un-chased given the marker-based backstop; documenting the limitation rather than silently living with it is the right call.

Style/conventions

Consistent with the file's existing very-verbose, reasoning-first comment voice and the TryXxx-never-throws helper pattern already used by TryCheckExportTarget/TryHardenExportedSecret/TryDescribeLinkTarget. CHANGELOG entry follows the established [Unreleased] > Fixed format with a bottom link-ref.

No correctness, security, or performance issues found beyond the one cosmetic note above.

[erikdarlingdata]

CI: all four checks green, mergeable: MERGEABLE, mergeStateStatus: CLEAN

build                    SUCCESS   3m3s
check-branches           SUCCESS   2s
review                   SUCCESS   3m52s
Darling PostgreSQL tests SUCCESS   3m16s

Checked mergeable immediately after opening (MERGEABLE, so CI actually ran) and again after settle - still MERGEABLE / CLEAN, so nothing on dev has moved under the CHANGELOG entry yet. Three sibling PRs are landing tonight; if one merges first this will need a keep-both-sides resolve on CHANGELOG.md and nothing else, since the two code files are touched by no other PR in flight.

On the review bot's one non-blocking note

It flags a TOCTOU gap between the File.GetAttributes probe and the File.Delete in TryRemovePartialExport, reasoning that a file vanishing in between would land in the generic catch (Exception cleanupFailure) and produce a misleading "left behind" warning.

The mechanism is not right: File.Delete is documented as a no-op for a path that does not exist and does not throw FileNotFoundException, so the race cannot reach that catch. The actual residual is the opposite and smaller - we would print Removed <path> for a file some other process removed a moment earlier, which is a true statement about the end state and a false one about who did it. On a single-process CLI cleanup path that is not worth a lock or a re-probe, and adding either would make the block less clear than the thing it guards against. Leaving as written.