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

CHANGELOG.md

@@ -111,6 +111,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - **Lite's portable ZIP is self-contained, which HALVED it** ([#2501]) - `Publish Lite` is now `-r win-x64 --self-contained` in both `build.yml` and `nightly.yml`, so neither Lite artifact has a .NET prerequisite any more and the failure [#2489] documented stops existing: a tester who unzips onto a stock Windows Server no longer meets the .NET host's bare `You must install .NET to run this application` before a line of our code runs. **The size went the opposite way from what bundling a runtime suggests.** The old publish was RID-agnostic, so it copied every platform its packages ship - **537 MB of `runtimes\` on a 565 MB tree** (osx 130, linux-x64 116, linux-arm64 70, win-arm64 56, then win-x86, musl, loongarch64 and riscv64), of which only the **52 MB `win-x64`** folder could ever load on Windows. `DuckDB.NET.Bindings.Full` is most of it, SkiaSharp and SqlClient behind it. Dropping ~485 MB of unloadable native payload beats the cost of bundling .NET, WPF and ASP.NET Core by roughly two to one: measured on one commit and one SDK, **565 MB tree / 212.7 MB zipped becomes 277 MB / 114.2 MB**. It matters most for the **nightly** ZIP, which is the UAT download and is not offered as a `Setup.exe` at all. **A RID-specific publish needed two more files than the flag.** `Lite/packages.lock.json` had only a `net10.0-windows7.0` target, and a RID restore adds `net10.0-windows7.0/win-x64` to it - after which the `dotnet restore --locked-mode` that BOTH workflows run before the publish fails `NU1004: the project's runtime identifiers have changed`, because locked mode compares the PROJECT's RID set (empty) against the lock file's (win-x64). Reproduced locally; that is a red CI run on every PR, not the future `--no-restore` trap it was filed as. The fix is `<RuntimeIdentifiers>win-x64</RuntimeIdentifiers>` in `PerformanceMonitorLite.csproj`, so the project itself asks for that graph and one committed lock file satisfies the RID-less locked-mode restore and the RID publish alike; `RuntimeIdentifiers` (plural) sets no RID on the build, so a plain `dotnet build` stays RID-agnostic and `Lite.Tests` is untouched. **SignPath needed nothing** - the `Lite` artifact-configuration slug already receives both shapes today, and the signed re-zip reads `signed/Lite/*`, inheriting whatever shape `publish/Lite` has. Auto-update is unaffected; the ZIP is not a Velopack channel. `LiteRuntimePrerequisiteDocsTests` went red on the flag alone (3 of its 7 facts) and was rewritten to state every claim BOTH ways round: [#2499]'s version asserted only that the docs DID name the runtimes, so two of its facts stayed green while the prose went stale. It now also derives the lock file's RID coverage from the `-r` flags in the workflows, and every new assertion was proven red with its fix reverted.
 
 ### Fixed
+- **Abandon-path forensics no longer record a session id of 0** ([#2884]) - every `ABANDONED` row V109 wrote carried `target_session_id = 0` - a value no real session has - while successful runs recorded real ids, and 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. The capture sat BEFORE `ExecuteReaderAsync`, and `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. And the write side guarded every OTHER forensic figure - `>= 0` on the row, byte and last-read counts, 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 capture now happens AFTER the open round trip: a drain-stall abandon (open completes in ~100-200ms; the [#2673] budget fires minutes into the read) therefore records its REAL id, which is exactly the case the column was built for, while an abandon that fires INSIDE `ExecuteReaderAsync` leaves it NULL - the declared NOT-RECORDED convention, and the honest answer for a connection that never finished its first exchange. `TryReadTargetSessionId` normalizes non-positive values to null at the source, and the writer grows the same-shaped guard as its sibling columns (`spid > 0`) - belt beside braces, because `DrainForensics` is a public record any caller can construct with a 0 the capture path would never produce. Pinned three ways, each proven red against the unfixed source: the capture must sit AFTER the `ExecuteReaderAsync` it depends on (a source-order pin, because the property is unmockable without a real `SqlConnection` and a refactor hoisting the capture back above the open reintroduces the bug while compiling clean), the helper's normalization, and the writer's guard.
 - **Every read in the alert evaluation pass now carries an explicit deadline** ([#2874]) - all forty-five commands across the six alert-pass types set no `CommandTimeout`, so every one inherited Npgsql's undocumented 30s default. On 2026-09-04 the forced-plan read failed five times on the production store, each surfacing as "Exception while reading from stream" - which is how Npgsql renders its OWN deadline, the misdiagnosis [#2826] exists to prevent. Unlike [#2810] and [#2871], which sit under `DarlingWorker.s_analysisTimeout`'s 120s `CancelAfter`, **this pass has no enclosing budget at all**: `EvaluateAlertsAsync` is called with the plain stopping token, so the per-command deadline IS the pass budget multiplied by the number of sequential reads - 45 x 30s of worst-case exposure while the body holds one of only four fleet sweep permits and that server's relaunch is skipped throughout. The value is therefore the first in this family set BELOW what it inherited rather than above: 10s, bounded under by the measured worst case (the shipped queries timed on the production store's three busiest servers put the whole pass's cost in one read - the forced-plan check at 1,744.9ms cold over ~6.0GB, every other read under 3ms) and bounded over by the 30s `s_alertSweepInterval` the pass runs on, so one stalled read still lets it finish inside the interval that restarts it. The asymmetry is what justifies erring short: an exceeded deadline skips one alert check and logs it, retrying 30s later, while a long-running read starves fleet-wide collection. Every observed failure was killed AT the 30s ceiling, so the record is right-censored - 10s is chosen from the measured cost and the cadence, not fitted to a failure distribution the data cannot show. Pinned structurally over both construction shapes: [#2874]'s census counted only `new NpgsqlCommand(`, missing `NpgsqlDataSource.CreateCommand(sql)`, which inherits the same default and accounts for four of this group's own sites - the repo-wide recount is recorded on the issue.
 - **A held Ctrl+V during a busy clipboard no longer spawns several 'Pasted Plan' tabs from one keypress** ([#2870]) - [#2837] moved the clipboard can't-open retry off a synchronous `Thread.Sleep` onto an awaited `Task.Delay`, which keeps the UI pump responsive but also yields the thread for the ~175 ms retry window. The old `Thread.Sleep` had incidentally serialized input, so a HELD Ctrl+V (OS key-repeat) could not dispatch a second paste until the first finished; with the async retry, on a transiently busy clipboard several paste KeyDown handlers can sit in their retry loops at once, and when the clipboard frees each succeeds and loads its own tab. Each paste surface now carries a `_pasteInProgress` re-entrancy flag: set synchronously before the awaited read and cleared in a `finally` that runs only after the load finishes, so it spans the clipboard read AND the off-thread plan parse (the loaders return `Task` and the paste handlers `await` them, not fire-and-forget) and a repeat paste arriving during either window is dropped (its key stays claimed via `e.Handled`). The same flag guards both the Ctrl+V handler and the Paste XML button on each surface, across all three front ends (Lite, the Darling viewer, and the deprecated Dashboard). A rare, benign burst that the [#2837] async change had newly made possible; source-pinned so the guard can't be dropped without failing the build.
 - **Retention held by the rollup-coverage gate is now visible, instead of reading as a healthy job** ([#2813]) - the #1680/#1877 gate PAUSES a retention policy so it cannot drop history a rollup has never materialized, which is correct and is not being weakened here. But a paused job is not a failing one: it reports `total_failures = 0`, `last_run_status = Success` and a plausible last-run duration, and `StoreSelfMetrics` never recorded `j.scheduled`, so every stored metric read clean. On the production store five `query_store_stats` policies sat held for **16 days** while the raw tier grew to 19 chunks and 65 GB - 4.5x its own 4-day horizon, and the store's largest object - and the only signal was one WARNING per service start ([#2809]). `get_store_metrics` now reports every held policy LIVE from the catalog, with the tier's actual span and how many times its configured `drop_after` it is really holding, and a `Retention Held` self-alert fires on the same hourly sweep as its [#2136] sibling. Judged on the CONJUNCTION of held AND past-horizon, never on the paused flag alone: `EnsureRetentionPoliciesAsync` deliberately creates every policy paused, so flag-only would fire on every fresh store at every start, and retention drops whole CHUNKS, so a 4-day policy with 1-day chunks legitimately sits at 1.25x while working perfectly. The ratio is deliberately not a hold duration - nothing records when a policy was paused - and measuring the consequence instead is what makes one threshold serve a 4-day raw tier and a 35-day baseline tier alike. Warning at 2.0x (clear of the 1.25x chunk floor, and ~8 days on the raw tier rather than the 16 it actually took), Critical at 4.0x, so the motivating incident reads Critical. The check never ACTS: arming a held policy drops the only copy of the history it holds, which is exactly what the gate prevents, so the release stays the `--backfill-rollups` operator decision and this makes the need for one visible rather than silent. Verified against a genuinely held state on TimescaleDB 2.28.1 - 19 chunks under a 4-day policy, measured 4.52x - by running the shipped reader itself, with the span normalization proven session-independent under UTC, UTC+14 and UTC-7. Persisting `scheduled` into the metrics series needs a migration rung and is tracked separately.
@@ -3206,6 +3207,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [#2828]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2828
 [#2837]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2837
 [#2870]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2870
+[#2884]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2884
 [#2864]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2864
 [#2874]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2874
 [#2876]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2876

Darling/Darling.Tests/CollectionLogDrainForensicsStoreTests.cs

@@ -226,12 +226,49 @@ public void TheUnmeasuredSentinelIsNeverWrittenAsALiteral()
         foreach (var guard in new[]
                  {
                      "drain.Value.RowsRead >= 0", "drain.Value.BytesRead >= 0", "drain.Value.LastReadMs >= 0",
+                     /* #2884: the session-id twin. 0 is the provider's not-populated state, never a real
+                        SPID or backend pid, and unguarded it wrote as a real-looking id no join could land. */
+                     "drain.Value.TargetSessionId is int spid && spid > 0",
                  })
         {
             Assert.Contains(guard, writer, StringComparison.Ordinal);
         }
     }
 
+    /// <summary>
+    /// The session id is captured AFTER the open round trip, exactly once, and the helper normalizes the
+    /// provider's not-populated 0 to null (#2884).
+    ///
+    /// <para>SqlConnection.ServerProcessId is not reliably populated until the connection has round-tripped
+    /// a command. The original capture sat before ExecuteReaderAsync, so the runs it mattered most for —
+    /// budget-abandoned cycles, where the id is the join key to waiting_tasks and the peer snapshots —
+    /// recorded a literal 0. Ordering is pinned on the source because it IS a source property: the capture
+    /// call must come after the ExecuteReaderAsync await it depends on, and a refactor that hoists it back
+    /// above the open reintroduces #2884 while compiling clean and passing every behavioral test that
+    /// cannot construct a real SqlConnection.</para>
+    /// </summary>
+    [Fact]
+    public void TheSessionIdIsCapturedAfterTheOpenRoundTrip()
+    {
+        var runner = ReadSource("Darling/PerformanceMonitor.Darling.Service/DarlingCollectorRunner.cs");
+
+        const string openCall = "opened = await command.ExecuteReaderAsync(";
+        const string captureCall = "context.TargetSessionId = TryReadTargetSessionId(";
+
+        var open = runner.IndexOf(openCall, StringComparison.Ordinal);
+        var capture = runner.IndexOf(captureCall, StringComparison.Ordinal);
+
+        Assert.True(open >= 0, "the server-scoped open call moved; re-anchor this pin");
+        Assert.True(capture >= 0, "the session-id capture is gone entirely");
+        Assert.True(capture > open,
+            "the session id must be captured AFTER ExecuteReaderAsync has round-tripped (#2884) — " +
+            "before it, SqlConnection.ServerProcessId reads 0 on exactly the abandoned cycles it exists to explain");
+        Assert.Equal(capture, runner.LastIndexOf(captureCall, StringComparison.Ordinal));
+
+        /* The helper's own normalization: 0 is not a session id and must become null at the source. */
+        Assert.Contains("return raw > 0 ? raw : null;", runner, StringComparison.Ordinal);
+    }
+
     /// <summary>
     /// A string read through the indexer counts once, not zero times (#2864 review). "A collector cannot
     /// forget to count" is this decorator's whole claim, and forwarding the indexer straight to the inner

Darling/PerformanceMonitor.Darling.Service/DarlingCollectorRunner.cs

@@ -1429,13 +1429,6 @@ it was. */
                    most, and the one a trailing assignment would leave at zero. */
                 serverPhasesMeasured = true;
 
-                /* #2864: the target's own session id, read off the OPEN connection as a property rather than
-                   asked for with SELECT @@SPID — a round trip here would be ~25,000 extra queries an hour
-                   across the fleet to fetch a number the client already holds. It is what makes a stalled run
-                   joinable to waiting_tasks / dmv_blocking_snapshot / query_snapshots, all of which record a
-                   session id; without it "what was OUR session waiting on" cannot be asked retrospectively
-                   even for a window where the answering snapshot was captured. */
-                context.TargetSessionId = TryReadTargetSessionId(targetConnection);
                 using var itemBudget = EnumeratedCollectorDriver.StartItemBudget(definition.PerItemWallClockBudget, cancellationToken);
                 var itemToken = itemBudget?.Token ?? cancellationToken;
                 try
@@ -1459,6 +1452,25 @@ misattribution the last time this was got wrong. */
                     }
 
                     using var reader = opened;
+
+                    /* #2864: the target's own session id, read off the connection as a client property rather
+                       than asked for with SELECT @@SPID — a round trip would be ~25,000 extra queries an hour
+                       across the fleet to fetch a number the client already holds. It is what makes a stalled
+                       run joinable to waiting_tasks / dmv_blocking_snapshot / query_snapshots, all of which
+                       record a session id; without it "what was OUR session waiting on" cannot be asked
+                       retrospectively even for a window where the answering snapshot was captured.
+
+                       Captured HERE, after ExecuteReaderAsync has returned, and not beside the phase flag
+                       above (#2884): SqlConnection.ServerProcessId is not reliably populated until the
+                       connection has round-tripped a command, so a read placed before the open recorded 0 —
+                       a value no real session has — on exactly the abandoned cycles the id exists to explain.
+                       Here the round trip has provably happened, so a drain-stall abandon (open completes in
+                       ~100-200ms; the budget fires minutes into the read) records its REAL id, which is the
+                       load-bearing case. An abandon that fires INSIDE ExecuteReaderAsync leaves this null,
+                       which the store reads as NOT RECORDED — the honest answer for a connection that never
+                       finished its first exchange, and one of the reachable-NULL cases the V109 write-side
+                       comment already documents. */
+                    context.TargetSessionId = TryReadTargetSessionId(targetConnection);
                     var drainWatch = Stopwatch.StartNew();
 
                     /* #2864: the collector reads through a counting decorator rather than the provider reader
@@ -1636,12 +1648,22 @@ failures would reproduce exactly the cross-referencing this change exists to rem
     /// <para>Best-effort by design. A provider that exposes no such property returns null, which reads as
     /// "not available" — never as a session id of 0, which is a real SPID.</para>
     /// </summary>
-    private static int? TryReadTargetSessionId(DbConnection connection) => connection switch
+    private static int? TryReadTargetSessionId(DbConnection connection)
     {
-        SqlConnection sql => sql.ServerProcessId,
-        NpgsqlConnection npgsql => npgsql.ProcessID,
-        _ => null,
-    };
+        /* A session id of 0 is never real — SQL Server assigns no SPID 0 to a user session, and both
+           providers report 0 from this property for a connection that has not completed the exchange
+           that populates it — so 0 is the provider's "not available" state, not a measurement. #2884
+           found it written to the store as though it were data, on exactly the abandoned cycles where
+           the id is the join key. Normalized to null here, at the source, so every consumer sees the
+           declared NOT-RECORDED convention instead of a real-looking placeholder. */
+        var raw = connection switch
+        {
+            SqlConnection sql => sql.ServerProcessId,
+            NpgsqlConnection npgsql => npgsql.ProcessID,
+            _ => 0,
+        };
+        return raw > 0 ? raw : null;
+    }
 
     /// <summary>
     /// Writes the per-item app-log lines for probe failures, capped at