Dotnet Hosted Service Review

dotnet-hosted-service-review

Reviewing a PR that adds or modifies a .NET BackgroundService / IHostedService (poll loops: background extraction, watchers, sync, sweep). Focus: robustness, cancellation, idempotency, registration, and logging invariants. Produces numbered findings with severity + file:line evidence and a claims-vs-code verdict.

Checklist

1. ExecuteAsync try/catch coverage — every await except the cancellation wait must be inside a try/catch (or have its own). The classic miss: the interval re-read / config reload AFTER the run body (timer.Period = await ReadIntervalAsync(...)) and the startup config read sit outside the try. An unhandled non-OCE exception there faults ExecuteAsync → default BackgroundServiceExceptionBehavior.StopHost stops the WHOLE host, contradicting any "best-effort" claim in the class doc. As of .NET 11 Preview 3 the cost is louder: RunAsync/StopAsync/WaitForShutdownAsync (and their sync forms) now fault instead of completing, so the process exits non-zero — a single failure rethrows, several combine into an AggregateException (Microsoft Learn). Evidence: read the loop body and check what is outside the try block, not just that a try exists.

2. Compare with the sibling hosted service in the same repo — same-repo precedent is the strongest review signal. If an existing service try/catches both its work body AND its delay, a new service that guards less is a finding. Cite the divergence explicitly, with file:line for both sides.

3. Cancellation handling in per-item catches — catch (Exception ex) per project/item swallows OperationCanceledException: on shutdown each in-flight item logs a spurious Warning and the top-level catch (OCE) when (IsCancellationRequested) break never fires from the run body. Inner catches should filter OCE (when (!cancellationToken.IsCancellationRequested)) or rethrow.

4. Timer semantics — PeriodicTimer first tick = one full interval: "enable" produces no work until the first tick (UX surprise; check for a run-now verb or a doc note). Re-reading the interval after each run is correct (config change without restart) but must be inside the try (item 1). timer.Period has a setter — mutation is fine.

5. Idempotency claims vs store semantics — "idempotent" claims must be checked at the store layer: SELECT-then-INSERT dedup (path-keyed) with NO unique constraint on the table = TOCTOU across processes. PRAGMA busy_timeout serializes commits, not read windows. Multi-process topology: each stdio MCP client spawns its own server process → N background loops over the same store when the feature is enabled. If the same exposure pre-exists on the synchronous tool path, rate it SHOULD-FIX (low), not MUST-FIX, and offer a partial UNIQUE index as the fix.

6. Settings-channel semantics — strict parsers (value == "true"), null/missing key → safe default (verify GetSettingAsync returns null for unset keys), unknown values → fail-safe mode. Grep SetSettingAsync callers to confirm the CLI is the only writer of the new keys. Case-sensitive parsing with fail-safe fallback is a NIT, not a bug.

7. Logging invariant — nested static partial Log, [LoggerMessage], explicit EventIds. Check collisions against whatever per-class EventId ranges the repo already reserves — read them out of the existing Log classes rather than assuming a numbering. EventIds are scoped per logger category, so cross-class reuse is benign — note only same-category collisions.

8. Safety claims → claims-vs-code table — turn every PR-body claim into a verdict row with file:line evidence (off-by-default / never-mutates / propose-never-shares / explicit-gate-warns / idempotent / no-delete-path). Grep for the forbidden operation (e.g. Delete* calls) to prove "no delete path" rather than trusting the diff. State verdict per claim: HOLDS / HOLDS-with-caveat / FAILS.

9. Test fidelity for loops — fakes that don't model store idempotency (fake ShareAsync appends unconditionally, shared index never updates after a share) make loop tests pass even if the loop re-does work every tick; the real cross-run idempotency (index re-read + store dedup) then has no test. Flag as NIT with the fix: mutate the fake's index on share.

Worked example

When reviewing, keep the repo's own past review records handy: finding shapes, evidence lines and the safety-claim table are easier to match than to invent, and citing a divergence from precedent lands better than citing a preference.

Gotchas

• Don't rate a pre-existing TOCTOU as MUST-FIX when the identical pattern already exists on the synchronous tool path — SHOULD-FIX (low) + owner question is the honest severity.
• BackgroundServiceExceptionBehavior.StopHost is the .NET default — verify the host config before claiming a faulted ExecuteAsync is contained. On .NET 11+ the host also surfaces the failure through RunAsync/StopAsync/WaitForShutdownAsync, so a faulted service turns a zero exit code into a non-zero one; a try/catch around the await host.RunAsync() is what restores the old silent exit.
• One host per process (the startup path picks a stdio host OR a web host, not both) means AddHostedService runs once per process — the multi-loop question is about processes sharing one store, not hosts within a process.
• Access-mode / auth guards usually gate only the MCP tool layer — a background service calling the store directly bypasses them. Raise as an owner question (intent), not an automatic finding.