Capture the target session id after the open round trip, and never write 0 (Fixes #2884)

[erikdarlingdata]

Fixes #2884.

The defect

Every ABANDONED row V109 wrote carried target_session_id = 0 — a value no real session has — while successful runs recorded real ids. The abandon path is the one place the id is load-bearing: it exists to join a stalled run to waiting_tasks / dmv_blocking_snapshot / query_snapshots.

Two defects compounded:

1. The capture sat before ExecuteReaderAsync. SqlConnection.ServerProcessId is not reliably populated until the connection has round-tripped a command, so a connection that had not yet completed that exchange reported the provider's not-populated 0.
2. The write side guarded every other forensic figure (>= 0 on rows/bytes/last-read, rejecting the in-memory -1 sentinel) but passed the session id through bare, so the 0 landed in the column as though it were data.

The fix

• Capture after the open round trip. A drain-stall abandon (open completes in ~100–200ms; the Bound each collector's per-server wall-clock footprint (tail is up to 555s on one server) #2673 budget fires minutes into the read) now records its real id — exactly the case the column was built for. An abandon that fires inside ExecuteReaderAsync leaves it NULL: the declared NOT-RECORDED convention, and one of the reachable-NULL cases the V109 write-side comment already documents. No new NULL-semantics claim is introduced.
• TryReadTargetSessionId normalizes non-positive values to null at the source — 0 is the provider's "not available" state for both SqlConnection.ServerProcessId and NpgsqlConnection.ProcessID, never a measurement.
• The writer grows the sibling-shaped guard (spid > 0) beside the existing >= 0 count guards, because DrainForensics is a public record any caller can construct with a 0 the capture path would never produce.

No schema change, no StorageVersion bump — the column exists; this is a write-path fix. No log line changed.

Verification

• Three pins, each proven red on the unfixed source, green on the fixed:
  • TheSessionIdIsCapturedAfterTheOpenRoundTrip — source-order: the capture must sit after the ExecuteReaderAsync it depends on, exactly once. Ordering is pinned on source because it is a source property: the property is unmockable without a real SqlConnection, and a refactor hoisting the capture back above the open reintroduces Abandon-path forensics record target_session_id = 0 instead of the real SPID (or NULL) #2884 while compiling clean.
  • The helper's normalization (return raw > 0 ? raw : null;).
  • The writer's guard, added to the existing TheUnmeasuredSentinelIsNeverWrittenAsALiteral guard set.
• Behavioral red/green on the built assembly (reflection harness, macOS net10.0): a never-opened SqlConnection — the same not-populated state the production rows captured — maps to null on the fixed helper and leaked 0 on the reverted one; the printed value changing across rebuilds also proves each rebuild took.
• Both projects build clean with EnableWindowsTargeting; no new warnings in touched files; CHANGELOG staged as 2 insertions / 0 deletions with a plain git add.
• Windows suites cannot run on macOS — CI is the arbiter for the xUnit runs.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MX6HyjsuDCs15qGB2rh4Gy

[claude]

Reviewed. This is a clean, narrowly-scoped fix and I did not find correctness, parity, security, or style issues.

Correctness

• The capture (context.TargetSessionId = TryReadTargetSessionId(targetConnection);) now sits after using var reader = opened; — i.e. strictly after ExecuteReaderAsync has round-tripped — which is exactly the fix the PR describes. Verified there's only one call site for the capture, so no stale copy was left behind.
• The ItemBudgetExpired catch block correctly returns context.TargetSessionId either way: null if the budget fired during the open (capture never ran), the real id if it fired during the drain (capture already ran). That matches the stated intent (NULL = "not recorded" vs. real id for drain-stall abandons).
• TryReadTargetSessionId's raw > 0 ? raw : null normalization is correct for both providers (SqlConnection.ServerProcessId / NpgsqlConnection.ProcessID both report 0 pre-round-trip; SPID 0 is never a real user session).
• The added spid > 0 guard in DarlingObservability.cs is redundant with the source-side normalization but is deliberately defense-in-depth since DrainForensics is a public record any caller could construct directly — consistent with the existing >= 0 guards on the sibling row/byte/last-read counters.
• Only one write path for target_session_id exists (InsertCollectionLogSql); no other call site needed the same fix.

Lite/Darling parity
No drift — the V109 drain-forensics columns (target_session_id included) are Darling-only by design, per the original #2864 changelog entry ("No Lite twin, deliberately... a DuckDB twin would be five forever-NULL columns"). Lite's collection_log table has no equivalent columns, so there's nothing for this PR to mirror.

Tests
TheSessionIdIsCapturedAfterTheOpenRoundTrip pins the capture call's source position relative to the ExecuteReaderAsync open call, and asserts it appears exactly once — a reasonable way to guard against a future refactor silently hoisting it back above the open (the property is genuinely unmockable without a real SqlConnection). The writer-guard addition to TheUnmeasuredSentinelIsNeverWrittenAsALiteral follows the existing pattern for the other three forensic fields.

Security / SQL
No T-SQL touched. The Postgres write is parameterized (NpgsqlParameter), no string concatenation of untrusted input.

No inline comments filed — nothing rose to that bar.