Skip to content

Report the store host's cloud instance type/VM size (#4214) - #4330

Merged
erikdarlingdata merged 6 commits into
devfrom
feature/4214-cloud-instance-identity
Sep 25, 2026
Merged

erikdarlingdata merged 6 commits into
devfrom
feature/4214-cloud-instance-identity

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 25, 2026 •

Copy link
Copy Markdown
Owner

Closes #4214.

Why

Part 2a (#4282) shipped get_store_host: the store host's platform, RAM, data volume, PostgreSQL/TimescaleDB
facts, and a verdict per sizing-relevant setting. This part (2b) adds the one fact that needed leaving the host
to answer: which cloud instance type or VM size the store is actually running on, so a sizing question never
needs a shell on the box just to run curl against the metadata endpoint by hand.

What changes

A new DarlingCloudIdentityProbe (Darling/PerformanceMonitor.Darling.Service/DarlingCloudIdentityProbe.cs)
probes the link-local instance metadata address (169.254.169.254) for an EC2 instance type or an Azure VM
size, and a CloudIdentity record struct (Provider, InstanceType) carries the result — both null when
nothing was found. It is wired into HostProfile as a new required Cloud field.

This is outbound HTTP from the service process, so the safeguards the ruling named are all in one file, load-bearing:

  • EC2 uses IMDSv2 only: a PUT /latest/api/token (with X-aws-ec2-metadata-token-ttl-seconds), then a
    GET /latest/meta-data/instance-type carrying that token. No IMDSv1 fallback — that would be a second,
    unauthenticated way to reach the same data.
  • Azure: one GET /metadata/instance/compute/vmSize?api-version=2021-02-01&format=text with Metadata: true.
  • A dedicated handler (DarlingCloudIdentityProbe.CreateHandler), never shared with any other outbound call
    in the process: UseProxy = false (a configured proxy never carries these requests) and
    AllowAutoRedirect = false (a redirect off the link-local address is never legitimate metadata, and is never
    followed).
  • Every request URL is a literal IP (169.254.169.254) — no DNS name is ever resolved for this probe.
  • 200 ms TOTAL for the whole probe, from one shared CancellationTokenSource, not a per-request timeout — the
    EC2 token PUT, the EC2 instance-type GET, and (only when EC2 found nothing) the Azure GET all share the one
    budget, so a slow first hop cannot spend it twice.
  • A 256-byte response cap: a body is read only up to that many bytes, then rejected as oversized.
  • A strict accept pattern on the final reported value only (^[A-Za-z0-9._-]{1,64}\z; \z, not $, so a trailing newline cannot pass) — deliberately NOT
    applied to the intermediate EC2 token, which legitimately carries +, /, = that pattern excludes.
  • Every failure the probe causes is "not found," never an exception: a timeout, a non-2xx status, a redirect,
    an oversized body, a pattern mismatch, a thrown exception — ProbeAsync catches all of it and returns
    CloudIdentity.None. The one exception is the caller's own cancellation, which propagates, so the host-profile
    cache never stores a "not found" for a caller that gave up.

Per the ruling, the probe reports only the provider (aws/azure) and the raw instance type/VM size string
— no disk-class mapping or other derived fact rides along; defaults over speculative config.

Called only from GatherAsync (--check-settings and get_store_host), never from
GatherStartupProfileAsync or anything DarlingWorker runs at service start — a host with no metadata endpoint
never pays this probe's cost on every collection cycle. get_store_host's existing 5-minute cache means one
probe per cache fill from that path. A source-text pin test proves both never-called claims against the actual
source (see Test plan), not just against today's behavior.

Surfaces (all three the ruling named, nothing more):

  • get_store_host's JSON payload: a new cloud: { provider, instance_type } object, and a short clause added to
    the tool's description tail (after <<GUIDE>>, so it does not touch the served tools/list head budget).
  • --check-settings' text output: a new Cloud: aws (t3.large) / Cloud: not detected line (never printed by
    the once-per-start log, which never probes — FormatStartupProfileText doesn't read the field at all). Also
    added cloudProvider/cloudInstanceType to --check-settings --json for parity with the text line (a small
    in-lane fix: the two output modes of the same command should carry the same facts).
  • The web panel (renderStoreHost/renderStoreHostPayload in sweeps.js): a new "Cloud" row between CPUs and
    RAM.

McpPayloadContractCensusTests and McpToolsListBudgetTests both passed unchanged — the new payload keys and
description text stayed under budget, so no pin file needed updating.

Four existing HostProfile object-initializer fixtures (DarlingFileLoggerProviderTests,
StoreHostProfileCacheTests, and StartupHostProfileLogTests, which now carries the same
"SHOULD-NEVER-PRINT" sentinel trick it already uses for Store) needed a Cloud value added once Cloud became
a required member — mechanical, no behavior change.

The production probe disposes the handler it creates (lane H2c).

Test plan

New Darling/Darling.Tests/DarlingCloudIdentityProbeTests.cs:

  • DarlingCloudIdentityProbeTests — driven entirely through a fake HttpMessageHandler, no real network
    call: EC2 token+instance-type success (and proves Azure is never reached once EC2 succeeds); EC2 401 falling
    back to Azure success; neither cloud responding (a handler that hangs until the probe's own 200 ms budget
    cancels it — asserted to return CloudIdentity.None well under a generous 2 s ceiling, not just eventually);
    EC2 404 with Azure also not found; an oversized (300-byte) EC2 body rejected; an EC2 redirect never followed
    (asserts exactly 3 requests total, proving the redirect target is never fetched); a value that fails the
    accept pattern.
  • DarlingCloudIdentityProbeSourcePinTests — source-text pins: GatherStartupProfileAsync and
    DarlingWorker.cs never reach DarlingCloudIdentityProbe; CreateHandler sets UseProxy = false and
    AllowAutoRedirect = false. Proved by hand that each of the four assertions fails when the source it pins
    is broken (temporarily flipped AllowAutoRedirect to true, and temporarily made
    GatherStartupProfileAsync call the probe; both broke their respective pin test; both reverted before
    committing, confirmed by a clean git status and a clean rebuild).

Ran (all green, Total: 240, Errors: 0, Failed: 0, Skipped: 4 — the 4 skips are the pre-existing
[Collection("live-postgres")] live tests this brief said not to stand up a rig for):

  • *StoreHost* (every DarlingStoreHostProfile*/DarlingMcpStoreHost*/StoreHostProfileCache* class)
  • DarlingCloudIdentityProbeTests, DarlingCloudIdentityProbeSourcePinTests
  • McpPayloadContractCensusTests, McpToolsListBudgetTests, McpToolTypeRegistrationTests,
    ReadmeDerivedCountPinTests (the MCP inventory/description-budget pin classes)
  • *DocCommentHygiene*
  • StartupHostProfileLogTests, DarlingFileLoggerProviderTests (the two other HostProfile fixture sites
    the new required member touched)
  • Re-ran the same set after git merge origin/dev (clean merge, no conflicts) — still green.
  • Build: dotnet build Darling/Darling.Tests/Darling.Tests.csproj — 0 Warning(s), 0 Error(s).
  • Full suite — not run; CI runs it.
  • The four live-postgres tests this touches indirectly (DarlingStoreHostProfileLiveTests,
    DarlingMcpStoreHostToolsLiveTests, DarlingMcpStoreHostBudgetLiveTests) — brief said start no rig; these
    construct a real HostProfile end-to-end through GatherAsync, so they'll now also exercise the probe
    against whatever 169.254.169.254 resolves to on the CI runner (typically nothing reachable, so a fast
    "not found" within budget) — CI's rig will confirm.

Round-1 review fixes

Round 1 is the security review in comment 5837132200 (no High, no Medium, eight Lows). All eight landed here (fix report: PR comment 5837689251):

  • Lows 2-6 and 8 (99404367): the handler bounds its connect attempt by the probe's 200 ms budget (ConnectTimeout), never drains an in-flight body when it is disposed (MaxResponseDrainSize = 0), and adds no trace headers (ActivityHeadersPropagator = null). The client caps a buffered body at 257 bytes, one past the 256-byte limit. A cancellation by the caller now propagates instead of reading as "not detected", so the host-profile cache never stores a wrong "not found". The value pattern anchors on \z, not $. The token TTL comment no longer calls 60 seconds the shortest TTL IMDSv2 accepts.
  • Low 1 (98e3342e): tests pin each safeguard on its own (the 256-byte cap, the status check, Azure's pattern check), and a call-site census over every .cs file in the service project replaces two name pins.
  • Low 7 (bf9e518b): the get_store_host description and DescribeCloud's doc comment say the value is what the metadata service reported, not verified.

CHANGELOG entry

SECTION: Added
ENTRY:

🤖 Generated with Claude Code

https://claude.ai/code/session_01FVjn4PBJN71NQXdFo6ZxNQ

erikdarlingdata and others added 3 commits September 25, 2026 13:20
Part 2b: the store host profile now reports the cloud instance type
(EC2) or VM size (Azure) alongside the existing platform/RAM/settings
facts, via a dedicated link-local metadata probe.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3
The one-argument ProbeAsync(CancellationToken) built a new SocketsHttpHandler
on every call and handed it to the two-argument overload, which constructs
its HttpClient with disposeHandler: false. Nothing disposed that handler, so
every probe (each --check-settings run and each get_store_host cache fill)
leaked one handler and its connection pool. The production path now owns
and disposes the handler it creates.

Pinned with a source-text assertion that the one-argument overload's body
contains "using var handler = CreateHandler()"; proven to fail against the
pre-fix expression-bodied overload.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3
@erikdarlingdata

Copy link
Copy Markdown
Owner Author

Round-1 security review at eaa64ee

Result: no High and no Medium findings. There are eight Lows. Three of them (Lows 2-4) come from SocketsHttpHandler runtime defaults, verified in-process on .NET 10. One (Low 1) is a set of tests that stay green when a named safeguard is removed. Nothing I found lets a request leave the link-local address, or reads a response without a bound. Nothing puts the token or a response body into a log or an answer, and no startup path reaches the probe.

Threat model. The probe runs inside the Darling service, from the service account's place on the network. I considered three attackers:

  1. A caller of get_store_host, through MCP or the web viewer, who wants to steer outbound requests or multiply them.
  2. Anything that answers at 169.254.169.254. That is the cloud's real metadata service, or, on a network outside a cloud, a device on the link or the default gateway. This attacker wants to put content into the outputs or tie up the service.
  3. A later edit that weakens a safeguard while CI stays green.

What I read.

  • DarlingCloudIdentityProbe.cs, all 208 lines.
  • The diffs of DarlingStoreHostProfile.cs, Mcp/DarlingMcpStoreHostTools.cs and sweeps.js.
  • StoreHostProfileCache.cs.
  • The --check-settings caller, DarlingCliCommands.cs:465-503.
  • The web read dispatch, DarlingWebEndpoints.cs:2954, and the two callers that build it with no config: DarlingTriageEndpoint.cs:563 and DarlingWebEndpoints.cs:1551.
  • el(), cellText and fmtText in the web code.
  • The logging setup of both web hosts.
  • DarlingCloudIdentityProbeTests.cs.

I also ran a scratch console app against the .NET 10 SocketsHttpHandler, configured like CreateHandler, with a ConnectCallback that returns an in-memory stream. No socket was opened and no metadata endpoint was called. I did not run the test suite, because this review is read-only and CI runs it.

Findings, all Low, most consequential first

Low 1: The tests do not pin seven of the named safeguards. Each one below can be removed with every test still green. (Darling/Darling.Tests/DarlingCloudIdentityProbeTests.cs)

  • The 256-byte cap (probe :199-202, test :117-139). The oversized test sends a 300-character body, and the pattern's {1,64} length limit rejects that body without any help from the cap. Delete the total > MaxResponseBytes check and the probe keeps 257 as. The pattern still rejects them, so the test still passes. Fix: send 60 valid characters followed by 240 spaces. That is 300 bytes, but after Trim() it would pass the pattern, so only the cap can reject it. Or send an oversized token, and assert that no instance-type GET is ever sent.
  • Rejecting non-2xx answers (probe :182-185, tests :63-86, :107-115 and :141-172). Every non-2xx fake is new HttpResponseMessage(status). Its default content is an EmptyContent of 0 bytes (confirmed). Delete the IsSuccessStatusCode check and every such body reads as empty, which also gives null, so no result changes. Fix: send a 404 and a 302 that each carry a body the pattern accepts, such as t3.large, and assert that both still give CloudIdentity.None.
  • The pattern on the Azure branch (probe :171). The only Azure success test (:63-86) sends a valid value, and no test sends a bad one. Delete ValuePattern.IsMatch(value) on line 171 and every test still passes. Without that check, a spoofed Azure answer of up to 256 bytes would reach the --check-settings text line, and ANSI escape codes could be part of it. Fix: add a test where the Azure answer is a value like bad value<script> and assert None.
  • No redirects (test :141-172). A fake HttpMessageHandler replaces SocketsHttpHandler, and auto-redirect lives inside SocketsHttpHandler. So setting AllowAutoRedirect = true would not change this test. Its count of 3 only proves that the probe's own code never requests the redirect target. The only real guard is the source pin at :270-277, which checks for the text anywhere in the file (a Contains over the whole file). The UseProxy pin at :261-268 works the same way. Fix: CreateHandler_ReturnsASocketsHttpHandler (:197-202) already builds the handler, so assert its properties (snippet below). That is deterministic, and neither reformatting nor a second handler elsewhere in the file can fool it.
  • One budget for the whole probe (probe :109-110, test :88-105). The hang starts at the first request, so the probe ends there and never reaches the Azure request. A per-request timeout of 200 ms, or even 1 s, would also finish inside the test's 2 s limit. Fix: pin that the file contains exactly one CancelAfter(. A timing test also works: the EC2 PUT answers 401 after about 190 ms and Azure hangs. The probe should then return in under about 300 ms, where one timeout per request would take about 390 ms.
  • Streaming the body (probe :141-143, :153-155 and :167-169). The fakes serve in-memory StringContent, so no test can tell a streamed read from a buffered one. Drop HttpCompletionOption.ResponseHeadersRead and HttpClient buffers the whole body before the cap check runs. The only limits left would be the 200 ms budget and the 2 GB default for MaxResponseContentBufferSize. Fix: set MaxResponseContentBufferSize = MaxResponseBytes + 1 on the client at :112, so even a buffered read stops at 257 bytes, and pin it.
  • Never called from startup (tests :241-259). These pins look for the text DarlingCloudIdentityProbe in the body of GatherStartupProfileAsync and in DarlingWorker.cs. But startup code would reach the probe through GatherAsync, not by name. A warm-up call from DarlingWorker, Program.cs or a host's start method would pass both pins, whether it calls DarlingStoreHostProfile.GatherAsync, StoreHostProfileCache.Shared.GetOrGatherAsync or DarlingMcpStoreHostTools.GetStoreHost. Fix: pin where the calls can appear anywhere in the service source:
    • DarlingCloudIdentityProbe.ProbeAsync( appears only in GatherAsync.
    • DarlingStoreHostProfile.GatherAsync( appears only in DarlingCliCommands.cs and Mcp/DarlingMcpStoreHostTools.cs.
    • GetStoreHost( appears only in the MCP tool and the web dispatch.

The pins that exist do fail on the exact reverts they target. I checked this by reading the code, and it agrees with the by-hand proof in the PR body. Two request-count checks also do real work. 2 at :60 proves that Azure is never asked once EC2 succeeds. 3 at :171 proves that the probe never requests the redirect target.

Low 2: The connection attempt outlives the 200 ms budget by about 5 seconds. (DarlingCloudIdentityProbe.cs:100-104)

The failure: take a host outside any cloud, on a network that silently drops connection attempts to 169.254.169.254. The budget cancels the PUT at 200 ms, and the probe returns None on time. But SocketsHttpHandler keeps the pending connection attempt alive for 5 s after the request that started it finishes (the PendingConnectionTimeoutOnRequestCompletion default). Disposing the handler does not stop it.

In my test run, the request returned at 214 ms, and the connection attempt was cancelled at 5,235 ms, after the handler had already been disposed. With ConnectTimeout = 200 ms, the attempt was cancelled at 211 ms.

The cost is one socket left trying to connect for about 5 s per probe. The single-flight cache limits this to one probe per 5-minute cache fill, plus one per --check-settings run, so a burst of calls cannot flood anything. Fix: ConnectTimeout = ProbeBudget.

Low 3: The 256-byte cap limits what the probe keeps, not what the socket reads. (DarlingCloudIdentityProbe.cs:199-202 with :100-104)

The failure: the responder answers 200 with a body of up to 1 MiB, either chunked or with a Content-Length header. The probe reads 257 bytes and returns null. But when the response is disposed, SocketsHttpHandler reads and discards the rest of the body so that it can reuse the connection. The runtime defaults allow up to 1 MiB (MaxResponseDrainSize) for up to 2 s (ResponseDrainTimeout), and that reading is not limited by the 200 ms budget.

In my test run, with a 500,000-byte body, the handler read all 500,000 body bytes, although the probe kept only 257. With MaxResponseDrainSize = 0, it read 4,053 bytes (one buffer) and closed the connection.

The extra bytes are thrown away, never stored, so the reading is bounded and memory is not at risk. It can still be up to 1 MiB for each of the probe's three responses. Response headers have their own limit, the 64 KiB default. Fix: MaxResponseDrainSize = 0.

Low 4: Which headers go out depends on tracing settings elsewhere in the process, not on this file. (DarlingCloudIdentityProbe.cs:100-104)

CreateHandler leaves ActivityHeadersPropagator at its default. Today no extra headers go out. Both web hosts call ClearProviders() (DarlingMcpHostService.cs:490 and DarlingWebHostService.cs:780), and nothing subscribes an ActivitySource or a DiagnosticListener. So ASP.NET Core creates no Activity per request, and the Azure request carries only Host and Metadata: true.

The failure comes if tracing is turned on later: a logging provider on either host, OpenTelemetry, or a diagnostic listener. A caller then sends traceparent, tracestate and baggage headers, and the request's Activity takes them in. The runtime then copies all three onto every metadata request.

In my test run, with an Activity current, the Azure GET carried traceparent, tracestate and baggage, with the caller's own tracestate and baggage values. With ActivityHeadersPropagator = null, it carried only Host and Metadata.

Even then, nothing secret would leak, only values the caller chose and random IDs. Still, the rule of Metadata: true and nothing else should be guaranteed by this file itself. Fix: ActivityHeadersPropagator = null.

Low 5: The catch-all also swallows the caller's cancellation, and the wrong result is then cached. (DarlingCloudIdentityProbe.cs:125-130 and StoreHostProfileCache.cs:66-95)

The failure: the store reads use up almost all of the McpStoreHostProfileSeconds deadline, and the deadline fires during the probe. The probe returns None instead of throwing, and GatherAsync returns a normal profile. The shared cache then serves cloud: not detected to every MCP and web caller for 5 minutes, even on a host that is in a cloud.

This is a wrong fact, not a leak. Fix: add catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { throw; } before the bare catch, so that only the probe's own 200 ms budget turns into "not found".

Low 6: ValuePattern ends in $, which in .NET also matches just before a trailing newline. (DarlingCloudIdentityProbe.cs:86)

Today the Trim() at :204 removes the newline before the check, so nothing gets through. But the protection lives in a different method from the check.

In my test run, the current pattern matched t3.large followed by a line feed, and the same pattern ending in \z did not. If the trim ever changes, a spoofed value could end in a newline and break the "Cloud:" line of --check-settings (DarlingStoreHostProfile.cs:878). Fix: ^[A-Za-z0-9._-]{1,64}\z.

Low 7: No one can check the reported value. Whatever answers at 169.254.169.254 chooses it, as long as it fits the pattern.

The failure: on a network outside a cloud, a device on the link or the gateway answers the token PUT and the instance-type GET. get_store_host, --check-settings and the web panel then report aws (<any value that fits the pattern>). An operator or an MCP agent sizing the store may treat it as fact.

The pattern stops injection, and no setting is computed from this field. So the harm is only a wrong fact on display, and plain-HTTP metadata always carries that risk. Fix (documentation only): say in the tool description, and next to the Cloud: line, that the value is what the link-local metadata service reported and has not been verified. Checking the cloud's signed identity documents would cost far more than this risk is worth.

Low 8: The comment on the token TTL is wrong. (DarlingCloudIdentityProbe.cs:67-70)

The comment calls 60 "the shortest TTL IMDSv2 accepts", but IMDSv2 accepts any TTL from 1 to 21,600 seconds. The PUT, the X-aws-ec2-metadata-token-ttl-seconds header name and the value 60 are all valid. Fix: correct the comment. You could also ask for a shorter TTL, since the token is used once, within 200 ms.

Suggested handler, client and test changes (Lows 1-4):

internal static HttpMessageHandler CreateHandler() => new SocketsHttpHandler
{
    UseProxy = false,
    AllowAutoRedirect = false,
    ConnectTimeout = ProbeBudget,       // Low 2
    MaxResponseDrainSize = 0,           // Low 3
    ActivityHeadersPropagator = null,   // Low 4
};

// in ProbeAsync(handler, ...)
using var client = new HttpClient(handler, disposeHandler: false)
{
    Timeout = Timeout.InfiniteTimeSpan,
    MaxResponseContentBufferSize = MaxResponseBytes + 1,   // Low 1, streaming item
};

// test: replaces the two whole-file text pins
using var created = DarlingCloudIdentityProbe.CreateHandler();
var h = Assert.IsType<SocketsHttpHandler>(created);
Assert.False(h.UseProxy);
Assert.False(h.AllowAutoRedirect);
Assert.Equal(0, h.MaxResponseDrainSize);
Assert.Null(h.ActivityHeadersPropagator);
Assert.True(h.ConnectTimeout <= TimeSpan.FromMilliseconds(200));

The six questions

1. Request handling. No request can leave the address. Two Lows: 4 and 8.

  • All three URLs are const literals that point at 169.254.169.254 (:59-62). HttpClient has no BaseAddress.
  • Nothing reads configuration, an environment variable or DNS. There is no ConnectCallback and no Host override.
  • UseProxy = false skips both a configured proxy and the proxy environment variables.
  • AllowAutoRedirect = false, and a 3xx answer fails IsSuccessStatusCode.
  • The URLs are http, so no Alt-Svc or HTTP/3 upgrade can apply.
  • The IMDSv2 PUT and its TTL header are correct. Low 8 is only about the comment.
  • The token stays in a local variable. It is sent once, in X-aws-ec2-metadata-token, on the GET to the fixed instance-type URL. It is never logged (the file has no logger), cached, returned, or sent to Azure. So a hostile token can only travel back to the address it came from.
  • The Azure GET carries Metadata: true. Low 4 describes the one way that header set could grow.

2. Response handling. Nothing from a response can be injected into any output. Two Lows (3 and 6) and one integrity finding (Low 7).

  • ResponseHeadersRead plus a fixed 257-byte read buffer means the probe itself never buffers a whole body.
  • The pattern is checked against exactly the string that is returned, after Trim().
  • The provider is a constant (aws or azure). The value can only contain [A-Za-z0-9._-] and is 1 to 64 characters long.
  • The MCP answer (DarlingMcpStoreHostTools.cs:118-122) and the --json output (DarlingStoreHostProfile.cs:932-933) both go through System.Text.Json.
  • The web row (sweeps.js:462) uses cellText. That calls el({ text }), which sets textContent, and el() throws on an html property.
  • No log line prints Cloud. FormatStartupProfileText never reads it, and the new sentinel in StartupHostProfileLogTests guards that.

3. Time and startup. No startup path reaches the probe. One Low: 2.

  • One linked cancellation source with CancelAfter(200 ms) (:109-110), with Timeout = InfiniteTimeSpan, covers the PUT, both GETs and every body read.
  • The URLs are IP literals, so there is no DNS lookup. The request's wait for a connection is covered too. Low 2 is about the connection attempt that keeps running in the background.
  • ProbeAsync has one caller, GatherAsync (DarlingStoreHostProfile.cs:757).
  • GatherAsync has two callers: --check-settings (DarlingCliCommands.cs:491) and get_store_host (DarlingMcpStoreHostTools.cs:95), through the shared cache.
  • The web read dispatch (DarlingWebEndpoints.cs:2954) calls that same tool with that same cache.
  • Triage and custom-view validation build the dispatch with no config. For them, get_store_host returns "unavailable" before it reaches the cache.
  • Nothing in the worker, startup, sweeps, alerts or collectors refers to the probe or to GatherAsync.
  • The cost on a host outside a cloud is at most about 200 ms per cache fill. The cache lets one caller through at a time (a SemaphoreSlim) and keeps results for 5 minutes. A burst of MCP or web calls therefore costs one probe, plus one per --check-settings run. No flood is possible.

4. Failures. No exception text or response body reaches any output. One Low: 5.

  • Every exception inside the probe becomes None, and the file does no logging.
  • Because the probe never throws, probe exception text cannot reach the FormatError in get_store_host (:174). It also cannot reach the CLI's "Could not read the store host profile" message.

5. Resources. Nothing leaks. There are two tails, both limited in time: Lows 2 and 3.

  • Each probe creates its own handler, and the production overload disposes it (:93, pinned).
  • The client is created with disposeHandler: false, and the client itself is disposed.
  • Every request and response is in a using, the stream is in an await using, and the cancellation source is disposed.
  • Repeated calls create one short-lived connection pool per probe. Through MCP or the web, that is at most one per 5 minutes.

6. Tests. See Low 1. The source pins fail on the exact reverts described in the PR body. The fake-handler tests do not protect five safeguards: the cap, rejecting non-2xx answers, the pattern on the Azure branch, the single budget, and the streamed read.

erikdarlingdata and others added 3 commits September 25, 2026 14:20
…4214 round 1, Lows 2/3/4/5/6/8)

CreateHandler now sets ConnectTimeout = ProbeBudget (a cancelled connect no longer keeps retrying for its own
default timeout after disposal), MaxResponseDrainSize = 0 (disposal never reads/discards up to 1 MiB) and
ActivityHeadersPropagator = null (tracing can never add traceparent/baggage to a metadata request). The
HttpClient caps MaxResponseContentBufferSize at MaxResponseBytes + 1 as defense in depth alongside the existing
manual streamed cap. A new catch clause lets the caller's own cancellation propagate as OperationCanceledException
instead of collapsing into CloudIdentity.None, so StoreHostProfileCache's single-flight gather (which only caches
a gather that returns normally) never caches a wrong 5-minute "not found" for a caller that gave up mid-probe.
ValuePattern is anchored with \z instead of $, since $ also matches just before a trailing newline. The TTL
comment no longer claims 60 seconds is the IMDSv2 minimum (it accepts 1 to 21,600).

Tests: extended CreateHandler_ReturnsASocketsHttpHandler to assert the five handler properties directly, in
place of the two whole-file text pins for UseProxy/AllowAutoRedirect. Added a test proving an already-cancelled
caller token throws through both ProbeAsync and StoreHostProfileCache.GetOrGatherAsync without caching anything,
and a test proving ValuePattern rejects "t3.large\n". Reverted the catch filter and the pattern anchor in turn
and confirmed each new test fails on the old shape, then restored both.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3
…round 1, Low 1)

Adds tests that isolate each safeguard from the value that would otherwise let it hide: a 60-char valid value
padded to 300 bytes with trailing spaces (passes ValuePattern once Trim()'d, so only the 256-byte cap can be
rejecting it), a 404 and a 302 that each carry the body "t3.large" (passes the pattern, so only IsSuccessStatusCode
can be rejecting it), and an Azure value that fails ValuePattern (the existing pattern test only exercises the EC2
side's own check). The existing oversized/404/redirect tests all used bodies that would already fail on other
grounds even with the safeguard under test removed, so none of them can catch its removal.

The three "never called from startup" name pins (checking one named method body, or one named file, for the
ABSENCE of a string) are replaced with a call-site census: every .cs file under
Darling/PerformanceMonitor.Darling.Service/ is scanned for each of three markers
(DarlingCloudIdentityProbe.ProbeAsync(, DarlingStoreHostProfile.GatherAsync(, GetStoreHost() and the set of files
that contain it must equal exactly the set ruling 3 names. A name pin only rules out the one spot someone thought
to check; a new call site anywhere else would pass it with everything green.

A Stopwatch-based timing test for "one shared budget, not a fresh timeout per request" was tried and dropped:
measured directly, it is racy. The shared outer budget still bounds the EC2 leg's own delay regardless of what
Azure gets, so under load EC2's delay can lose the race to that shared budget before completing "naturally" and
the assertion passes for a reason unrelated to what it meant to check (measured 2 of 3 runs still passing against
a deliberately broken, per-request-timeout build). Replaced with a deterministic source pin instead, exactly as
the brief's fallback names: the file must contain exactly one CancelAfter( call, since a per-request-timeout
implementation needs a second one.

Reverted each new safeguard (the cap check, the IsSuccessStatusCode check, Azure's ValuePattern.IsMatch, the
single CancelAfter() and, for the call-site census, a second ProbeAsync( call site added to DarlingWorker.cs) one
at a time and confirmed each corresponding new test fails, then restored all of them.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3
get_store_host's tool description and DescribeCloud's doc comment both now say the instance_type/VM size is
whatever the link-local metadata service reported, not verified against anything this host actually runs on.
McpToolsListBudgetTests (5 tests) still passes without changing TotalCeilingBytes, so no constant or change-log
line update was needed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TszxYhJJbTEh4LrZ56NYo3
@erikdarlingdata

Copy link
Copy Markdown
Owner Author

Round-1 fixes done. Three commits on feature/4214-cloud-instance-identity, head now bf9e518b. No advisor call, no plain-English pass, per the brief.

1. Harden the probe (Lows 2, 3, 4, 5, 6, 8) — commit 99404367

Darling/PerformanceMonitor.Darling.Service/DarlingCloudIdentityProbe.cs:

  • CreateHandler (line ~113): added ConnectTimeout = ProbeBudget, MaxResponseDrainSize = 0, ActivityHeadersPropagator = null.
  • ProbeBudget (line ~74): changed private to internal so the test can assert ConnectTimeout against the same constant instead of a duplicated magic number.
  • The client (line ~130): added MaxResponseContentBufferSize = MaxResponseBytes + 1.
  • The catch (line ~148): added catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { throw; } before the bare catch, so only the caller's own cancellation propagates; the probe's own 200 ms budget expiring still collapses to CloudIdentity.None. Confirmed by reading StoreHostProfileCache.GetOrGatherAsync (StoreHostProfileCache.cs line ~66): it only assigns _entry after gather returns normally, no catch, so this exception reaches its caller uncached.
  • The pattern (line ~90): $ to \z.
  • The TTL comment (line ~68): no longer claims 60 s is IMDSv2's minimum (it accepts 1 to 21,600 s).

Darling/Darling.Tests/DarlingCloudIdentityProbeTests.cs:

  • Extended CreateHandler_ReturnsASocketsHttpHandler to assert UseProxy, AllowAutoRedirect, ConnectTimeout, MaxResponseDrainSize, ActivityHeadersPropagator on the built handler; removed the two whole-file text pins (CreateHandler_SetsUseProxyFalse, CreateHandler_SetsAllowAutoRedirectFalse) it replaces.
  • Added ProbeAsync_CallerTokenAlreadyCancelled_ThrowsAndCacheStoresNothing: proves the probe throws OperationCanceledException for an already-cancelled caller token, and that routing the same exception through StoreHostProfileCache.GetOrGatherAsync leaves nothing cached (a follow-up call still re-gathers).
  • Added ValuePattern_RejectsATrailingNewline: "t3.large\n" against ValuePattern directly.

Revert-proved both: reverting the catch filter to when (false) failed the cancellation test ("No exception was thrown"); reverting the pattern to $ failed the newline test ("Match found"). Restored both, rebuilt clean.

Test run: DarlingCloudIdentityProbeTests + DarlingCloudIdentityProbeSourcePinTests, 13/13 passed.

2. Pin the safeguards round 1 could remove (Low 1) — commit 98e3342e

New tests in DarlingCloudIdentityProbeTests.cs, each isolating one safeguard from a value that would otherwise slip through it:

  • ProbeAsync_Ec2InstanceTypeBodyExceedsCapByTrailingWhitespace_RejectedByTheCapAlone_ReturnsNone: 60 valid characters + 240 spaces (300 bytes). Reverting the cap check (if (total > MaxResponseBytes) to if (false)) let it through as aws/60 a's — confirmed, then restored.
  • ProbeAsync_Ec2InstanceTypeIs404WithAValidLookingBody_RejectedByStatusNotBody_ReturnsNone and the 302 counterpart: both carry body "t3.large". Reverting IsSuccessStatusCode (to if (false)) let both through as aws/t3.large — confirmed both fail, then restored.
  • ProbeAsync_AzureValueFailsAcceptPattern_ReturnsNone: Azure answers "bad value<script>". Reverting TryAzureAsync's own ValuePattern.IsMatch call let it through as azure/bad value<script> — confirmed, then restored.

Replaced the two startup name-pins (GatherStartupProfileAsync_NeverReachesTheProbe, DarlingWorker_NeverReachesTheProbe) with a call-site census (ProbeAsyncCallSite_OnlyInsideDarlingStoreHostProfile_GatherAsync, GatherAsyncCallSite_OnlyInCliCommandsAndTheMcpStoreHostTool, GetStoreHostCallSite_OnlyInTheMcpToolAndTheWebDispatch) scanning every .cs file under Darling/PerformanceMonitor.Darling.Service/ for each marker and asserting the exact file set ruling 3 names. The old pins each checked one named spot for the absence of the probe's name; the census instead checks that the marker's file set equals the named set exactly, so it also catches a new call site in any third file the old pins never looked at, not just the two they named. Revert-proof: added a throwaway private method calling DarlingCloudIdentityProbe.ProbeAsync( inside DarlingWorker.cs — the census failed as expected (["DarlingStoreHostProfile.cs", "DarlingWorker.cs"] vs expected ["DarlingStoreHostProfile.cs"]). Removed the throwaway method afterward; git diff on DarlingWorker.cs is empty.

The budget/timing item needed a course correction. I wrote a Stopwatch-based test per the brief (EC2 401 at ~190 ms, Azure hangs, bound at 300 ms between a correct ~200 ms and a broken ~390 ms), reverted the shared budget to a per-request one to prove it, and got an inconsistent result: 1 of 3 runs failed as expected, 2 of 3 passed anyway. Root cause: the shared outer budget still bounds the EC2 leg's own 190 ms delay regardless of what Azure gets, so under load that delay can lose the race to the 200 ms outer budget before finishing "naturally," short-circuiting the whole probe at ~200 ms for a reason unrelated to whether Azure's budget is shared or fresh. Per the brief's own fallback, replaced it with a deterministic pin instead: CreateHandler_ArmsExactlyOneBudget_NotAPerRequestTimeout asserts the source contains exactly one CancelAfter( call. Revert-proved by adding a second CancelAfter( call site (Azure's own fresh budget) — failed deterministically (Expected: 1, Actual: 2) on all runs; restored.

Test run: full DarlingCloudIdentityProbeTests + DarlingCloudIdentityProbeSourcePinTests, 19/19 passed. StoreHostProfileCacheTests, 4/4 passed.

3. Say the value is unverified (Low 7) — commit bf9e518b

  • Mcp/DarlingMcpStoreHostTools.cs (get_store_host description, in the cloud (...) clause): added "the value is whatever the link-local metadata service reported, not verified".
  • DarlingStoreHostProfile.cs, DescribeCloud's doc comment (line ~838, its only call site is the Cloud: line at ~880): added the same idea. DescribeCloud has exactly one caller in the whole project, confirmed by grep.
  • McpToolsListBudgetTests: 5/5 passed unchanged — the added text did not push get_store_host's description over TotalCeilingBytes, so no constant or change-log line update was needed.

Test totals

  • DarlingCloudIdentityProbeTests + DarlingCloudIdentityProbeSourcePinTests: 19/19.
  • DarlingStoreHostProfileTests: 50/50.
  • StoreHostProfileCacheTests: 4/4.
  • McpToolsListBudgetTests: 5/5.
  • DocCommentHygiene*: 77/77.
  • Every build: Darling.Tests.csproj -c Debug, 0 Warning(s), 0 Error(s) (one xUnit2008 warning appeared mid-edit from an early Assert.False(regex.IsMatch(...)), fixed to Assert.DoesNotMatch before committing).
  • Full suite and Lite: not run — not asked for; CI runs the full suite, and get_store_host is confirmed Darling-only (pinned in Lite.Tests/CrossAppMcpToolInventoryPinTests.cs), so no Lite parity work applies here.

For the coordinator to double-check

  • The judgment call in item 1: making ProbeBudget internal (was private) so the test can assert ConnectTimeout == ProbeBudget directly rather than a hardcoded 200. Small, mechanical, in the file the brief named.
  • The course correction in item 2: the Stopwatch timing test specified in the brief was measured flaky and replaced with the brief's own named fallback (a CancelAfter( count pin). The flakiness and the fix are both in the 98e3342e commit message.
  • I did not touch round 1's other Low findings beyond what the brief listed, and did not read the round-1 review itself, per the brief.

No new issues filed — everything fit inside the three listed items.

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