Extract the App Insights source, watermark and persistence ports; fix the culture-formatted KQL window (#374, #398)

[sambetts]

Summary

Finishes #374. Part 1 (PR #390) extracted the pure window rule; this puts the importer's remaining I/O behind ports, so the orchestration itself — the day loop, the window it derives, the per-day failure isolation — runs in a unit test with zero HTTP and zero SQL Server.

It also fixes a real, silent, production bug found while writing the culture test the task called for: #398. That change is deliberately separate from the extraction and is called out below.

Ports

WebJob.AppInsightsImporter.Engine/Interfaces.cs:

Port	Replaces
IAppInsightsSourceLoader	ai.GetPageViewsFromAppInsights / GetCustomEventsFromAppInsights
IHitWatermarkStore	db.hits.OrderByDescending(h => h.hit_timestamp).Take(1)
ISiteFilterLoader	SiteFilterLoader.Load(db)
IImportDbMaintenance	ImportDbHacks.CleanDuplicateHitsAndCreateIX_PageRequestID(db)
IAppInsightsDayPersistenceManager	pageViewsResult.SaveToSQL(...) / events.SaveAllEventTypesToSql(...)

Sql/SqlAppInsightsAdapters.cs holds the four SQL adapters. Each wraps the original query or call verbatim and borrows the caller's AnalyticsEntitiesContext rather than creating one, so the whole run keeps using the single context it has always used — creating a context per day would change both the change-tracker lifetime and the connection churn.

IImportDbMaintenance is also named in #369; it is defined once, here. #369 part 2b will use this definition rather than adding a second.

IAppInsightsDayPersistenceManager is deliberately coarse — one method per thing the day loop saves. #369 decomposes the SQL side further (page-views, searches, clicks, page-updates, hit-updates); those become collaborators of the SQL adapter, not changes to this port.

AppInsightsAPIClient

• Implements IAppInsightsSourceLoader via thin GetPageViewsAsync / GetCustomEventsAsync aliases over the existing method names, so no existing call site or test changes.
• Gained a constructor overload taking an HttpMessageHandler. That is what makes the retry, back-off, transient classification and token-refresh logic testable. The handler is passed with disposeHandler: false so a caller-owned stub is not disposed by the client; the production constructor is unchanged in behaviour (new HttpClient { Timeout = 10 min }).

AppInsightsImporter

Takes the five ports plus IAnalyticsDbContextFactory (#368), all optional trailing parameters — no call site breaks. When every database-backed port is supplied it runs without creating a context at all; otherwise it opens one exactly as before and builds SQL adapters over whichever ports were not supplied. The day loop moved into its own method so it can be driven by either the injected source or the production API client.

Every log message, its wording and its ordering is unchanged. Two shape-only differences worth a reviewer's eye:

• newestHit != null became newestHitTimestamp.HasValue, and the log interpolates newestHitTimestamp.Value where it used to interpolate newestHit.hit_timestamp. Same rendered text (hit_timestamp is a non-nullable DateTime on the entity, and Nullable<DateTime>.ToString() on a value-bearing instance is identical to the underlying value's).
• The API client is now disposed after jobTimer.TrackFinishedEventAndStopTimer rather than just before it, because the using moved out one level. It is an HttpClient with no outstanding requests at that point.

Separate change: fixes #398 — the KQL window was culture-formatted

GetWhereString built the per-day KQL window with a culture-sensitive ToString("yyyy-MM-dd HH:mm:ss"). The current culture selects the calendar, not just the separators:

Culture	new DateTime(2026, 5, 30) renders as
en-US / invariant	2026-05-30 00:00:00
th-TH (Thai Buddhist)	2569-05-30 00:00:00
ar-SA (Umm al-Qura)	1447-12-13 00:00:00

(verified on this machine, not assumed). KQL todatetime() only understands Gregorian ISO dates, so on such a host the importer asks App Insights for a window centuries away, gets an empty result set rather than an error, logs 0 page-views, 0 events for every day, and completes "successfully" — forever, with nothing in the logs explaining why the tenant has no hits data.

Fixed by formatting with InvariantCulture, with AppInsightsClient_BuildsKqlWindow_ForRequestedDayInUtc_UnderAnyCulture pinning it across th-TH, ar-SA and en-US.

Note the window rule (AppInsightsImportWindow) is not affected — .NET date arithmetic is tick-based and Gregorian, so culture is a no-op there. This is exactly the distinction PR #390 drew: a culture test is theatre where nothing formats, and meaningful where something does. Here something does.

Found by the review loop: a real regression, now fixed

The first review round caught an actual behavioural defect in this PR — the first the loop has found across five PRs, so it is worth calling out.

Extracting the day loop into ImportDaysAndSave changed what the download catch''s Release-only return did. It used to return out of ImportAndSave entirely, so jobTimer.TrackFinishedEventAndStopTimer never ran; after the extraction it returned only out of the inner method, and control resumed in ImportAndSaveWith and emitted FinishedSectionImport — reporting a successful section import for a cycle that downloaded nothing. Invisible in DEBUG, which rethrows.

ImportDaysAndSave now returns false on that path and the caller skips the completion event. Guarded in both build configurations by AppInsightsImporter_FatalDownloadFailure_DoesNotReportTheSectionAsFinished (DEBUG asserts the throw, Release asserts the return; both assert the event is absent), paired with AppInsightsImporter_SuccessfulRun_DoesReportTheSectionAsFinished so the fix cannot be "achieved" by never emitting the event at all.

Two test-quality findings from the same round were also applied:

• The retry tests no longer measure wall-clock time. They inferred back-off from elapsed time, which also measures scheduling, GC and deserialisation — flaky under CI load, and capable of passing after a no-back-off regression. AppInsightsAPIClient now has an internal RetryDelay function (defaulting to Task.Delay) and the tests assert the delays actually requested: 1s / 2s / 4s for exponential back-off, and 7s twice when the server sends Retry-After: 7.
• The injected-clock guard covered only half the call site. Every orchestration test passed daysBeforeOverride: null, and ResolveOverrideStartUtc ignores its clock argument in that case, so the first _clock.UtcNow read in ImportAndSave was unguarded. Added AppInsightsImporter_DaysBeforeOverride_IsAlsoResolvedFromTheInjectedClock.

Testing

New: 16 tests, zero HTTP, zero SQL Server, zero wall clock.

AppInsightsApiClientTests (8) — against a stub HttpMessageHandler:

• a 503 is retried, and the exponential back-off actually requested is 1s / 2s / 4s;
• the back-off doubles and then caps — asserted on the extracted pure BackoffSecondsFor(attempt), because the retry loop stops at MaxRetries (16s) and can never reach the 60s cap, so a loop-driven test would stay green if Math.Min were deleted;
• Retry-After: 7 is preferred over exponential back-off — asserted as two 7s waits where the exponential path would have asked for 1s then 2s;
• a 403 is not retried (retrying a missing API permission five times just delays the error the operator needs);
• transient failures are capped at MaxRetries + 1 attempts and the failure then surfaces, so an App Insights outage cannot hang the web-job;
• a token inside the 5-minute refresh margin is re-fetched, and the new token reaches the Authorization header;
• a still-valid token is not re-fetched per request (an Entra ID round-trip per API call at scale);
• the KQL window is Gregorian under any culture (above).

AppInsightsImporterOrchestrationTests (8):

• the whole import runs on fakes: startup maintenance runs once (not per day), the watermark is read once, the requested days match the window, only days with data are saved, and the org-URL filter loaded at startup is the same instance handed to the save;
• the window is driven by the injected clock — the clock is fixed years in the past, so no wall-clock-derived window could produce those days. This closes the gap AppInsightsImportWindowTests documented ("nothing invokes ImportAndSave, so replacing those reads with DateTime.Now would leave every test green"); that docstring is updated in this PR;
• an empty database scans the 31-day fallback window;
• one day failing to save does not abort the remaining days — every day is still fetched and the other two are still saved;
• a run with no data touches persistence zero times rather than running the merge SQL on empty collections every cycle.

Pre-existing tests unchanged and passing, including AppInsightsKqlGetWhereStringTests, AppInsightsImportWindowTests, AppInsightsAuthTests, AppInsightsImportTests, HitImportFanoutTests and DuplicateUrlTests. Area run: 107 passed / 1 failed of 108 — the failure is the pre-existing CommentsCognitiveTests, which cannot resolve the placeholder cognitive endpoint locally and passes in CI. Full solution builds (Debug).

Reviewer hotspots

1. The two-branch structure in ImportAndSave — is the "all database ports supplied" fast path genuinely equivalent to the adapter path, and does the context still live exactly as long as it did?
2. Log fidelity — particularly the newestHitTimestamp.Value interpolation and the moved using noted above.
3. disposeHandler: false — correct for a caller-owned stub, but confirm the production path (which passes null and uses the plain HttpClient constructor) still owns and disposes its own handler.
4. The #if DEBUG in AppInsightsImporter_FatalDownloadFailure_DoesNotReportTheSectionAsFinished — it is there because the production path it guards is itself #if DEBUG-forked, and CI runs the suite in both configurations. Is there a cleaner way to cover both, short of removing the fork?

Addresses #374
Addresses #398