feat(aspire): actionable diagnostics when resources fail to start

[thomhurst]

Why

When an Aspire resource fails to start under TUnit, the failure was hard to diagnose: the fixture only read State.Text, so a user got an opaque "Resource 'x' failed to start" / timeout plus raw logs — no exit code, no health-check reason, no clue which dependency was blocking, and the state-transition timeline went only to stderr and was lost.

This makes the failure itself answer what's wrong and what to do next, and routes the detail into the test report.

What changed (all in TUnit.Aspire.Core/AspireFixture.cs)

• Snapshot-driven diagnosis — on failure/timeout, read the live CustomResourceSnapshot (ExitCode, HealthReports, state) and classify the cause: container-runtime-down, image-pull failure, port-in-use, OOM (exit 137), non-zero exit, health-check-failing, never-started, crashed-no-code. Each class maps to one actionable hint.
• State distinction at timeout — "Running but not Healthy (health 'ready' Unhealthy: connection refused)" vs "still Starting (never reached Running)" vs "FailedToStart, exit code 137 (OOM)", derived from each resource's current snapshot.
• WaitFor dependency chain — turns the opaque "Waiting" into "api waiting on 'migrations' (Exited, exit code 1)" via WaitAnnotation.
• Startup timeline — the background monitor records state/health transitions and folds them into the diagnostics; its lifetime now spans both StartAsync and the resource-wait phase so health-wait hangs are covered.
• Concise exception + attached artifact — the exception message stays short (diagnosis + hints + pointer); the full timeline and untruncated per-resource logs are written to an attached Artifact (on the failing test, with TestSessionContext fallback), so it shows up in trx/HTML/IDE.

Incidental fixes

• Add Exited to the fail-fast watch — a container that crash-exits cleanly never reports FailedToStart, so previously we waited the full timeout for an already-dead resource.
• Lock LogProgress — the background monitor, the main init thread, and multiple fixture instances all write the shared static stderr stream; without a lock, lines splice.
• Fix LogLine rendering — $" {line}" emitted the noisy generated record ToString() (LogLine { LineNumber = .. }); now uses .Content (also in WatchResourceLogs).

Notes

• No public API changes — every addition is private/internal. Both source-gen and reflection modes; Native-AOT safe (public property reads only, no member reflection).
• Every Aspire 13.4.6 API used was verified by reflection-probing the actual assembly. Key gotcha encoded in the design: HealthStatus is null unless state == Running, so diagnosis reads HealthReports directly.

Testing

• 18 pure no-Docker unit tests in TUnit.Aspire.Tests/AspireDiagnosticsTests.cs (classification, hints, descriptions, log-signature scan, timeline formatting) — all pass.
• Builds clean on net8.0 / net9.0 / net10.0.
• Docker-backed integration fixtures (bad image tag, exit-1, never-healthy) are a sensible follow-up; not included here.

[codacy-production]

Not up to standards ⛔

🔴 Issues 1 medium

Alerts:
⚠ 1 issue (≤ 0 issues of at least minor severity)

Results:
1 new issue

Category	Results
ErrorProne	1 medium

View in Codacy

🟢 Metrics 21 complexity

Metric	Results
Complexity	21

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[claude]

Code Review

This is a well-designed improvement to Aspire startup diagnostics. The separation of pure classification logic (Classify, ScanLogSignatures, HintFor, DescribeState) from side-effectful I/O makes the code testable and the 18 unit tests are exactly the right kind to have here. The real bug fixes (LogLine .Content, stderr locking, Exited in fail-fast, monitor lifetime spanning both phases) are all valid. A few concerns worth raising:

---

High: Exited in fail-fast may trigger false failures on short-lived resources

WaitForResourcesWithFailFastAsync now watches for both FailedToStart and Exited:

string[] failureStates = [KnownResourceStates.FailedToStart, KnownResourceStates.Exited];

If a short-lived resource (e.g. a DB migration runner or seeder) exits successfully with code 0, it may transition to Exited state — and would immediately trigger the failure path, even though it succeeded. In Aspire, Finished is typically used for cleanly-exited executable resources, but Exited can be ambiguous. A failed test output saying "Resource 'migrations' failed to start" for a seeder that ran fine would be very confusing.

Suggested improvement: Check exit code before treating Exited as a failure, or only fail-fast on Exited when the exit code is non-zero/null:

// Inside the failure-detection task:
await notificationService.WaitForResourceAsync(name, evt =>
    evt.Snapshot.State?.Text == KnownResourceStates.FailedToStart ||
    (evt.Snapshot.State?.Text == KnownResourceStates.Exited && evt.Snapshot.ExitCode is not (null or 0)),
    failureCts.Token);

This preserves the intent (catch a crash-exited container) without catching a successfully-completed job.

---

Medium: Sequential log collection compounds timeout on multi-resource failures

BuildDiagnosticsAndAttachAsync collects logs for each problem resource sequentially:

foreach (var name in problemResources)
{
    var logLines = await CollectResourceLogLinesAsync(app, name);   // 5s timeout each
    ...
}

With 3 failing resources, that's up to 15 seconds added to an already-failed test's teardown. Consider collecting all resources in parallel:

var logsByName = await Task.WhenAll(problemResources.Select(async name =>
    (name, logs: await CollectResourceLogLinesAsync(app, name))));

---

Medium: SummarizeDependencies reports only the first dependency

The concise exception message (SummarizeDependencies) exits on the very first wait annotation, so a resource waiting on multiple dependencies only shows one in the short message. The full artifact (via AppendDependencyChain) correctly shows all. Consider returning a comma-joined summary or the first unresolved dependency specifically:

// Show all pending deps instead of just the first
var pendingDeps = waits
    .Where(w => /* dep not yet in ready state */)
    .Select(w => FormatDep(w))
    .ToList();
return pendingDeps.Count == 0 ? null : string.Join(", ", pendingDeps);

---

Low: Timeline cap drops the oldest (most diagnostic) transitions

RecordTransition trims from the front when the timeline exceeds 200 entries:

while (_timeline.Count > 200 && _timeline.TryDequeue(out _))
{
}

For a flapping resource, this drops the initial startup transitions that show what happened first — the most useful information for diagnosing a cascading failure. If bounding is needed, consider keeping the first N and last N entries instead of a pure sliding window.

---

Low: FormatTimeline silently returns empty on single transitions

if (transitions.Length < 2) return string.Empty;

A single recorded state transition (e.g. resource immediately jumped to FailedToStart) is still useful to see. The < 2 guard loses this. The concern about "nothing to show" doesn't apply when there's one real transition; the guard makes more sense as == 0.

---

Low: DateTime.Now in artifact filenames — use UtcNow

var fileName = $"aspire-diagnostics-{typeof(TAppHost).Name}-{DateTime.Now:yyyyMMdd-HHmmss-fff}.log";

CI runners typically run in UTC, but DateTime.Now is timezone-dependent. Using DateTime.UtcNow makes artifact names consistent across environments.

---

Low: Bare catch {} is too broad

TryWriteDiagnosticsArtifactAsync catches all exceptions including OutOfMemoryException and ThreadAbortException. The comment ("diagnostics must never mask the underlying startup failure") is correct in spirit, but prefer catch (Exception ex) with at least a Debug.WriteLine or Trace.Write so the failure isn't entirely silent during development:

catch (Exception ex) when (ex is not OutOfMemoryException)
{
    // diagnostics must never mask the underlying startup failure
}

---

Nice-to-have: HealthSignature is per-resource but doesn't include report names

The health signature used for change detection is just the worst-status string ("Unhealthy", "Healthy", etc.). If two health checks for the same resource flip independently but cancel out at the aggregate level (one gets better, one gets worse), the change is invisible. For the timeline this is probably fine (it'll detect most changes), but a fuller signature like string.Join(",", reports.OrderBy(r => r.Name).Select(r => $"{r.Name}:{r.Status}")) would be more precise.

---

Summary

The PR's architecture is solid — the pure-function/side-effect separation is clean, the artifact attachment approach is practical, and the real bug fixes are correct. The main concern is the Exited fail-fast behavior, which could cause confusing false failures for short-lived successful resources. The other issues are incremental quality improvements.

[thomhurst]

Thanks for the review — addressed in 6b36d0a. Verified each against the code; all were valid:

• High — Exited false-positives: switched the fail-fast watcher to the predicate WaitForResourceAsync overload; Exited now only counts as a failure with a non-zero exit code (IsFailureState), so a one-shot resource that exits cleanly (code 0) is no longer mis-reported.
• Medium — sequential log collection: logs for all problem resources are now gathered with Task.WhenAll; output order stays deterministic.
• Medium — only first dependency shown: SummarizeDependencies now skips already-ready deps and reports the first unsatisfied one (the full chain remains in the artifact via AppendDependencyChain).
• Low — timeline cap dropped the startup story: now keeps head and tail (first 80 + last 120), dropping only the middle churn.
• Low — FormatTimeline hid single transitions: guard relaxed to == 0; a lone immediate-failure transition now renders.
• Low — DateTime.Now: → DateTime.UtcNow for the artifact filename.
• Low — bare catch {}: now catch (Exception ex) and logs the artifact-write failure via LogProgress instead of swallowing it.
• Nice-to-have — HealthSignature: now a per-check name:status signature, so independent checks flipping in opposite directions still register and the timeline shows which check moved.

19 unit tests pass; builds on net8.0/net9.0/net10.0.

[claude]

Code Review: feat(aspire): actionable diagnostics when resources fail to start

Overview

This PR transforms opaque Aspire startup failures into actionable diagnostics. The core design — classify → hint → describe → attach — is clean and well-structured. The 18 unit tests covering the pure logic are a good foundation, the bug fixes (log line ToString(), StdErr locking, Exited fail-fast) are all correct, and the parallel log collection via Task.WhenAll is the right call.

No blocking issues. A few things worth noting:

---

Issue 1: Artifact filename collision in parallel test runs

File: AspireFixture.cs — TryWriteDiagnosticsArtifactAsync

var fileName = $"aspire-diagnostics-{typeof(TAppHost).Name}-{DateTime.UtcNow:yyyyMMdd-HHmmss-fff}.log";

Two fixture instances with the same TAppHost type that time out within the same millisecond will race to write the same path. One will silently overwrite the other's artifact, and the first test's artifact pointer will point at corrupted/wrong content.

Suggested fix:

var fileName = $"aspire-diagnostics-{typeof(TAppHost).Name}-{DateTime.UtcNow:yyyyMMdd-HHmmss-fff}-{Guid.NewGuid():N}.log";

This is low-probability but the fix is one token and eliminates the class of error entirely.

---

Issue 2: Double TryGetCurrentState lookup for the same resource

File: AspireFixture.cs — DiagnoseResource and AppendResourceDiagnosticBlock

Both methods independently call notificationService.TryGetCurrentState(name, ...). The snapshot is fetched twice, which is fine if TryGetCurrentState is a cheap dictionary lookup, but it also means the two calls could observe different state if a transition happens between them (unlikely during diagnostics, but technically possible).

Suggested refactor: Capture the snapshot once in DiagnoseResource and thread it through to AppendResourceDiagnosticBlock. The state divergence risk is low, but the architectural clarity is higher — the diagnostic block should be rendering a snapshot, not re-querying live state mid-render.

// In ResourceDiagnosis record, add the snapshot:
internal readonly record struct ResourceDiagnosis(
    string Name,
    string State,
    int? ExitCode,
    ResourceFailureClass Class,
    string? Detail,
    string? Hint,
    ResourceEvent? Event);  // carry the event for downstream rendering

---

Issue 3: Timeline reads can race with drain/refill during the failure path

File: AspireFixture.cs — RecordTransition and BuildDiagnosticsAndAttachAsync

The comment on RecordTransition says "reads happen after it stops", but FormatTimeline() is called from BuildDiagnosticsAndAttachAsync, which is called from within the try block — before StopMonitorAsync runs in the finally. This means _timeline.ToArray() (in FormatTimeline) can execute concurrently with the drain-refill loop:

Thread A (monitor): TryDequeue loop draining → queue empty → not yet re-enqueued
Thread B (timeout): FormatTimeline → _timeline.ToArray() → sees []  ← stale empty snapshot
Thread A: re-enqueues head + tail

The result is a timeline that shows as empty in the diagnostic output for the failure case. Since this is diagnostic-only and ConcurrentQueue.ToArray() is itself thread-safe, this won't cause any crash or incorrect exception. But it means the most useful artifact (the timeline showing what states the resource cycled through) can silently be empty exactly when it matters most.

Suggested fix: Stop the monitor before calling BuildDiagnosticsAndAttachAsync, or simply call StopMonitorAsync before building diagnostics in the failure paths. The small delay (waiting for the monitor task to observe the cancel) is worth the guarantee that the timeline is complete.

---

Issue 4: RecordTransition trimming is untested

The head/tail compaction logic in RecordTransition is the most algorithmically novel part of the PR and has a non-obvious invariant (the just-enqueued item must end up in the tail). This is the one piece of code that isn't covered by the 18 unit tests. A test with > 400 transitions asserting that the head (first N) and tail (last N) survive compaction would be a good addition.

---

Strengths worth calling out

• Classification pipeline (Classify → HintFor → DescribeState) is the right abstraction — it's pure, testable, and composable. The unit tests validate each layer independently.
• Parallel log collection with Task.WhenAll is the correct approach; sequential collection with per-resource 5s timeouts would compound badly.
• Exited fail-fast fix (only triggering on non-zero exit) is a meaningful correctness improvement — migration runners and seeders that exit cleanly were wrongly mis-classified before.
• StdErr locking is a real bug fix; the race was always there with multi-fixture runs.
• line.Content bug fix is a clear correctness improvement.
• Artifact fallback to TestSessionContext is a thoughtful touch for when there's no active TestContext.

Overall this is solid work. The issues above are all low-severity relative to the value the PR delivers.

[thomhurst]

Second review pass addressed in d21fab6 — all four were valid:

• Issue 1 (filename collision): added -{Guid.NewGuid():N} to the artifact filename, so same-TAppHost fixtures timing out in the same millisecond can't overwrite each other.
• Issue 2 (double TryGetCurrentState): the snapshot is now read once per resource in BuildDiagnosticsAndAttachAsync and the ResourceEvent is threaded through DiagnoseResource and AppendResourceDiagnosticBlock — no more re-querying live state mid-render.
• Issue 3 (timeline read/drain race): good catch — BuildDiagnosticsAndAttachAsync runs while the monitor is still recording (it's stopped in InitializeAsync's finally). Switched _timeline from ConcurrentQueue to a List guarded by a lock; RecordTransition and FormatTimeline both take the lock, so a read can no longer observe a transient empty/partial queue mid-rebuild.
• Issue 4 (untested trimming): extracted the compaction into a pure internal static TrimTimeline(list, head, tail) and added two tests (400→200 keeping head+tail; under-cap left unchanged).

21 unit tests pass; builds on net8.0/net9.0/net10.0.

[claude]

Code Review — Third Pass

All issues from both previous review rounds have been correctly addressed. Here's a verification pass on each fix, plus a look at the final state.

---

Previous Issues — All Resolved ✅

Round 1 fixes (6b36d0a):

• Exited false-positives: IsFailureState correctly gates on snapshot.ExitCode is not (null or 0) — migration runners exiting cleanly with code 0 are excluded. ✅
• Sequential log collection: now Task.WhenAll over all problem resources. ✅
• SummarizeDependencies now skips ready deps and surfaces the first unresolved blocking dependency. ✅
• Timeline cap: head/tail compaction (first 80 + last 120) instead of pure sliding window. ✅
• FormatTimeline guard relaxed from < 2 to == 0 — single transitions now render. ✅
• Artifact filename: DateTime.UtcNow ✅
• Bare catch {}: now catch (Exception ex) with LogProgress surfacing the failure. ✅
• HealthSignature: now a per-check name:status string, so independent checks flipping in opposite directions both register. ✅

Round 2 fixes (d21fab6):

• Artifact filename collision: {Guid.NewGuid():N} added — same-TAppHost fixtures can't race to the same path. ✅
• Double TryGetCurrentState: snapshot is fetched once in BuildDiagnosticsAndAttachAsync and threaded through to both DiagnoseResource and AppendResourceDiagnosticBlock as a parameter — no mid-render re-query. ✅
• Timeline read/drain race: _timeline is now a plain List<StateTransition> guarded by _timelineGate; both RecordTransition and FormatTimeline() take the lock, giving FormatTimeline a consistent snapshot via _timeline.ToArray() under the lock. ✅
• TrimTimeline extracted as internal static and covered by two unit tests (400→200 with head+tail preserved; under-cap unchanged). ✅

---

Final State: Clean

The architecture is sound end-to-end:

• Pure classification pipeline (Classify → HintFor → DescribeState) — all tested independently, no I/O dependencies.
• StateTransition record is correctly defined as internal readonly record struct with positional constructor, accessible via using static in the test file.
• AppendResourceDiagnosticBlock receives the pre-fetched ResourceEvent? ev as a parameter alongside the ResourceDiagnosis — a cleaner alternative to embedding the event in the record struct that keeps the diagnostic model free of raw Aspire API types.
• IsFailureState gate is precise: FailedToStart (always) or Exited with non-zero/non-null exit code (crash exit without FailedToStart marker). Clean one-shot resources are unaffected.
• 21 unit tests covering classification, hints, descriptions, log-signature scanning, timeline formatting, and timeline compaction — all pure-function tests with no Docker dependency.

No new issues found. This is ready to merge.