Skip to content

On Azure SQL Database, the long-query trace runs only in monitored databases and no longer drops a session another registration still uses - #4944

Merged
erikdarlingdata merged 17 commits into
devfrom
fix/long-query-banner-azure-db
Oct 2, 2026
Merged

erikdarlingdata merged 17 commits into
devfrom
fix/long-query-banner-azure-db

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Oct 2, 2026 •

Copy link
Copy Markdown
Owner

What changes

On Azure SQL Database, the opt-in long-query trace uses a database-scoped Extended Events session. This PR changes where Lite and Darling create and drop that session, and it corrects the text that describes it. It also widens where Darling's --drop-xe-sessions searches for that session to the databases the server excludes. An excluded database the verb cannot open is reported in a note and does not fail the command. On every other engine the session is server-scoped, and that path does not change. The always-on deadlock and blocked-process sessions do not change either.

Each fact in this body is observed in the code or in a test, unless it says "documented" (Microsoft Learn, linked below) or "inferred".

The rule is in one place, LongQueryTraceDatabases in PerformanceMonitor.Collectors, so both apps compute the same sets:

  • While the trace is on, the app creates the session in each monitored database. It drops the session from each listed database outside that set.
  • While the trace is off, the app drops the session from every listed database. The exclusions and the database scope do not apply.
  • A database that is also monitored as its own server belongs to that registration. The logical server's registration does not create, drop or read the trace there.
  • No drop removes the session from a database where another registration of the same logical server has the trace on and keeps the session. See "Registrations that share a session" below.
  • The app never creates or drops the trace in master.

The monitored set in each app:

The trace is opt-in because of its overhead, so it must not run in databases that the user left out. The always-on sessions keep their inventory rule (#3477). One list provisions both of them, and each of their collectors can have its own scope. A comment beside the always-on listing in DarlingXeSessions.cs names both rules.

When the apps apply the rule

  • While the trace is on, Lite creates the session where it is missing, in each monitored database, on every cycle. It runs the drop pass after a start, and again when the state key changes.
  • Darling runs the whole reconcile after each connect, and again when the state key changes. While the trace is on, it also runs the create side once an hour. See "A session dropped from outside" below.
  • The state key (LongQueryTraceDatabases.StateKey) holds the on or off state, the separately monitored databases, and the other registrations that keep the session. While the trace is on, it also holds the database scope and the exclusions. A change to any of them runs the plan again, so a newly excluded or out-of-scope database loses its session.
  • Each part of the state key now has its own separator. Before, a scope of alpha and beta with gamma excluded gave the same key as a scope of alpha with beta and gamma excluded. That change did not run the plan again, so beta kept its session.

Failures and retries

  • If the database listing fails, the reconcile throws. The app does not mark the reconcile done, so the next cycle lists the databases again.
  • On dev, Darling marked the trace applied after a failed listing while turning it on. No session was created until the next reconnect. The RED test On_AFailedListing_IsNotMarkedApplied showed this.
  • If a drop fails in one database, the app still tries every other database. Then it throws one LongQueryTraceDropException that names each database that failed, and the next cycle tries again.
  • Lite now logs each failed drop at Warning, not Debug, as Darling does.
  • After 5 failed cleanup passes in a row under one state key, the app stops retrying on every cycle. It marks the reconcile done and logs one Warning that names each database where the session can remain.
  • After that, the app tries the cleanup again once an hour. A failure logs at Debug, both the failed database and the pass, so the Warning is not repeated. The clock lives in LongQueryTraceDropRetry, and each app passes the time in, so the tests drive it.
  • A cleanup pass that succeeds resets the count and ends the hourly attempts. So do a change of the state key, a restart, and a Darling reconnect. Turning the trace on or off changes the state key.
  • In Lite, the count now resets only after a cleanup pass that ran and succeeded. Before, every finished cycle reset it. While the trace is on, a cycle that skips the cleanup still finishes. With the old reset, that cycle ended the hourly attempt.
  • The cap covers the drop outside the set while the trace is on, and the drop while it is off. Without the cap, a database that always refuses the drop logs a warning on every cycle.
  • While the trace is on, a failed drop does not undo the create side. The collector reads the monitored databases as usual.
  • Each pass lists the databases again, so a database that is no longer listed is not tried again.

A session dropped from outside

  • Darling latches the reconcile. On dev, it runs again only after the next connect or a change of the state key. A session dropped outside the app stays gone until then.
  • The read cannot tell. The shared read (LongQueryCompletionsCollector.BuildQuery) joins sys.dm_xe_session_targets to sys.dm_xe_sessions, or their database-scoped forms, on the session name. An absent session gives zero rows and no error (observed in the code). So the read's error arm for errors 297 and 15151 never sees it, and there is no error on which to clear the latch.
  • Now, while the trace is on and the reconcile is latched, Darling runs the create side again once an hour. It creates the session where it is missing and drops nothing. ServerLoopState.LongQueryTraceAppliedAtUtc holds the time of the last pass that created the session. A reconnect resets it with the latch.
  • The drop side still runs only after a connect, after a change of the state key, or as the attempt after the cap.
  • The worker picks the pass: LongQueryTracePass.Full, CreateOnly or RetryAfterCap. Only the Azure SQL Database arm reads it, because the server-scoped arm has no drop while the trace is on. On other engines the hourly pass runs the same create check as a connect.
  • A create pass does not reset the cleanup's count, so it cannot end the hourly attempt after the cap.
  • Lite needs no change here: while the trace is on, it creates a missing session on every cycle.

Databases monitored as their own servers

The rule uses the existing helper AzureMasterScope.SeparatelyMonitoredDatabases. Lite calls it through KnownEngineEditions. Darling calls it with the live server registry, which is the same set that the alert sweep uses.

Both read loops now skip the same databases, for this collector only. The new member SkipsSeparatelyMonitoredDatabases is true only on LongQueryCompletionsCollector. Assume that the logical server's registration reads such a database while that database's own trace is off. Lite's read then fails there on every cycle (#4731), and Darling's read returns zero rows that look like a quiet trace. While that database's own trace is on, the events are stored twice.

The helper has two rules. For session ownership:

  • It counts only enabled registrations. This is correct. A disabled registration runs no lifecycle and no read, so its database goes back to the logical server's registration. That registration reads the session if its trace is on, and drops it if its trace is off.
  • It leaves out registrations with read-only intent. That is right for the read: a read-only replica runs its own copy of the session and captures its own events. It is not right for the drop. The session's definition is one object for the database. It is created on the primary and copied to each read-only replica. It cannot be created while connected to a read-only replica (documented). A drop on the primary removes it from the replicas too (inferred). On a tier with no read-only replica, every connection goes to the primary, whatever its intent (documented).

Source for the documented facts: Read queries on replicas. Its section "Monitor read-only replicas with Extended Events" covers the session's definition. Its introduction says that the Basic, Standard and General Purpose tiers have no replicas and no read scale-out. It also says that with read scale-out turned off, a connection reaches the primary whatever its ApplicationIntent.

Registrations that share a session

So the drop follows its own rule, LongQueryTraceDatabases.KeptElsewhere. A drop skips a database while another registration keeps the session there. That registration must be enabled, belong to the same logical server, and have the trace on. Its read-only intent does not matter. A registration keeps the session in these databases:

  • A registration of one database keeps the session in that database.
  • A logical server's registration keeps the session in each database where it creates it. That is each database inside its database scope, except master, the databases monitored as their own servers, and the databases that it excludes.

The rule applies to every drop: the drop while the trace is off, and the drop outside the set while it is on. It applies to a logical server's registration and to a registration of one database. Creates do not change.

The coverage is exact. Suppose two logical server registrations both exclude a database. Neither one keeps the session there for the other, so a session left there is still dropped.

The list of databases monitored as their own servers is the same for every registration of a logical server, including a registration of one database. Each app computes it as the logical server's registration sees it: KnownEngineEditions.SeparatelyMonitoredDatabasesOnServer in Lite, and DarlingWorker.LongQueryTraceServerSeparatelyMonitored in Darling. A registration's own list does not work here. For a registration of one database, that list is empty. So its own database counts as kept by the logical server's registration, which never creates the session there. The drop is then skipped, and the session stays.

Each app builds the registrations beside its list of separately monitored databases:

  • Lite: the configured servers with the same server name, each with its trace setting from the schedule (GetScheduleForServer) and its exclusions. Lite has no database scope.
  • Darling: the live registry entries with the same host, each with its trace setting from ResolveSchedule, its exclusions, and its scope from DatabaseScopeFor. The registry holds only monitored servers, so each one is enabled.

The state key holds the other registrations that keep the session (LongQueryTraceDatabases.CoOwners). A registration of one database adds its database. A logical server's registration adds its exclusions, its scope and the list of separately monitored databases. So when the other registration turns its trace off, or changes what it covers, the skipped drop runs on the next pass.

The two open cases from round 2 are closed

  • Read-only intent, no replica. The case: a registration of one database has read-only intent, on a tier with no read-only replica. Its connection reaches the primary (documented), so it shares the session with the logical server's registration. In both apps, the logical server's drops now skip that database while the read-only registration's trace is on. The read-only registration's own drop runs while its trace is off, which is the default. That drop now skips its database while the logical server's trace is on and covers it.
  • Two registrations of the same logical server through master. In both apps, each one's drops now skip the databases that the other one keeps. Lite accepts such a pair when the two differ in read-only intent, as the text of its duplicate check says. Darling accepts it too. ServerIdHelper.BuildStorageName adds :RO to the identity. The comment on ActualIdentityCollision says that a read-only-intent registration beside a read-write one is legitimate. So the tests for this case run in both apps.

One limit remains. A registration of one database with read-only intent and the trace on, connected to a read-only replica, cannot create the session there. Its trace works only while a registration on the primary keeps the session's definition (documented).

Can any path drop a session that holds unread events for a database that the user still monitors?

  • On, create: No. This path only creates. One older exception is unchanged. If the session exists but the login cannot see it, the create step drops it and creates it again for that login. The old session's events were invisible to that login. The code is RecreateDatabaseScopedXeSessionAsync in Lite and EnsureOneDatabaseScopedAsync in Darling.
  • On, drop outside the set: No. The database is outside this registration's set, it has no registration of its own, and no other registration keeps the session there.
  • Off, drop: Yes, for this registration's own databases, and that is the purpose of turning the trace off. The events since the last read are lost. That was also true on dev. A database that is monitored as its own server is skipped, and so is a database where another registration keeps the session.
  • The cap: No. The cap never drops anything. It slows the retries to one an hour, so a session can remain and keep filling its ring buffer, which SQL Server bounds.
  • The hourly attempt after the cap: The same drops as the pass it repeats, with the same sets and the same rules.
  • Darling's hourly create pass: No. It only creates.
  • Reconnect or restart: No extra drops. The plan uses the same sets as in steady state, from the same saved registrations. Darling publishes its server registry before it builds the collector runner, so the first reconcile already sees the separately monitored databases and the other registrations. The create path leaves a visible session in place, so its unread events stay readable.
  • Cross-registration: No. Each drop skips a database where another enabled registration of the server has the trace on and keeps the session. That holds in both directions, whatever the read-only intent.

Text

  • The banner on Azure SQL Database, in both apps: "Enabling it creates the Extended Events session in each monitored database. Disabling it drops the session from every database that has it, except where another registration of that database still has the trace on. A database that is also monitored as its own server follows that server's setting."
  • The Darling README: "On Azure SQL Database the session is database-scoped. Enabling it creates the session in each monitored database and drops it from any other database that has it. Disabling it drops the session from every database that has it. Neither drop removes the session from a database where another registration of that database still has the trace on. A database that is also monitored as its own server is left to that registration."
  • Lite's schedule description (ScheduleManager.cs, collection_schedule.json) and Darling's CLI help now say that on Azure SQL Database the session is created in each monitored database.
  • Each app still builds the banner from the target's engine edition, and LongQueryCompletionsCollector.SessionScopeSentence still holds its last sentences. Only the Azure SQL Database sentences above changed.
  • The warning after the cap has a new last sentence, and Darling adds one more:
    • "It tries again once an hour, and right away after a restart, a change to the trace's settings, or a change to another registration of these databases."
    • Darling only: "It also tries again after it reconnects."
  • Darling's --drop-xe-sessions prints a warning, and the README quotes it. Both now say that Darling creates a missing long query completions session again within an hour. Before, they said only "on its next connect".
  • Two comments on this PR asked for a space after = in ServerTab.LongQueries.cs and ViewerServerTab.LongQueries.cs. An earlier commit in this PR removed those spaces, and a later one put them back. At this head both lines match dev.

Out of scope

Lite and Darling use the same session name, so the two apps share one session in a database that both monitor. A separate change handles that.

Tests

The hourly attempt and the hourly create pass

The new tests pass a time to the reconcile, and Darling's tests pass a pass kind, so they cannot compile at 824ea3d. Commit 7e5c34e adds the clock, the pass kind and the tests, with the behavior of 824ea3d. At that commit the cap forgets its state when it gives up, and the warning keeps its old last sentence. Darling never picks the create pass. Commit 13dcb7a is the fix.

Lite, RED at 7e5c34e:

  • Off_AfterTheCap_TriesAgainOnceAnHour_AtDebug_WithNoSecondWarning: Assert.Contains() Failure: Sub-string not found. It fails first on the warning's last sentence. Its later checks: nothing runs within the hour, one attempt runs an hour after the cap, and the cycle after it runs nothing. A second failure an hour later logs at Debug too, and no Warning follows the cap's one.
  • AfterTheCap_AnAttemptThatSucceeds_EndsTheHourlyAttempts: Assert.Equal() Failure: Collections differ
  • On_AfterTheCap_TheCyclesInBetween_KeepTheHourlyAttempt: Assert.Equal() Failure: Collections differ

Darling, RED at 7e5c34e:

  • Off_AfterTheCap_TriesAgainOnceAnHour_AtDebug_WithNoSecondWarning: Assert.EndsWith() Failure: String end does not match. It makes the same checks as the Lite test, and checks Darling's reconnect sentence.
  • AfterTheCap_AnAttemptThatSucceeds_StartsTheCountAgain_AndEndsTheHourlyAttempts: Assert.Equal() Failure: Collections differ
  • On_ASessionDroppedFromOutside_IsCreatedAgainAnHourLater_AndNothingIsDropped: Assert.Contains() Failure: Item not found in set. The registration excludes gamma, so a pass that runs the drop side drops gamma again. The test checks that nothing is dropped.

Guard, which passes at both commits: On_WithinTheHour_TheLatchedReconcileRunsNothing (Darling). Within the hour, a latched reconcile lists nothing and touches no database.

Red-watch for Lite's reset: put the old reset back, so that every finished reconcile resets the count. Then On_AfterTheCap_TheCyclesInBetween_KeepTheHourlyAttempt fails, and the other 20 tests in its class pass. With the fix, all 21 pass.

Existing tests that changed:

  • PostgresEngineGateBehaviorTests (Darling): the session reconcile takes the pass kind, and the gate tests pass Full.
  • TheSweep_PassesTheRegistrationsAndTheServersOwnList_FromTheLiveRegistry (Darling): the sweep now passes DateTime.UtcNow to the reconcile.

Registrations that share a session

The new tests call members that do not exist at 7cc63c1, so they cannot compile there. Commit d1a8499 adds the registrations, KeptElsewhere, the co-owner part of the state key, and the tests. At that commit, KeptElsewhere and the co-owners count no other registration. The state key joins its parts as before. So the behavior is the same as at 7cc63c1. Commit 464e314 is the fix.

Lite, RED at d1a8499:

  • Off_LeavesTheDatabaseOfAReadOnlyRegistration_WhileItsTraceIsOn
  • ReadOnlyRegistration_Off_LeavesItsDatabase_WhileTheLogicalServersTraceIsOnAndCoversIt
  • Off_LeavesTheDatabasesOfASecondLogicalServerRegistration_WhileItsTraceIsOn
  • On_TheOutsideDrop_LeavesAnExcludedDatabase_ThatASecondRegistrationsTraceCovers
  • Off_OnceTheOtherRegistrationTurnsItsTraceOff_TheNextPassDrops
  • TheLiteBanner_OnAzureSqlDatabase_SaysEachMonitoredDatabase_NotThisServer

Darling, RED at d1a8499:

  • Off_LeavesTheDatabaseOfAReadOnlyRegistration_WhileItsTraceIsOn
  • ReadOnlyRegistration_Off_LeavesItsDatabase_WhileTheLogicalServersTraceIsOnAndCoversIt
  • Off_LeavesTheDatabasesOfASecondLogicalServerRegistration_WhileItsTraceIsOn
  • On_TheOutsideDrop_LeavesAnExcludedDatabase_ThatASecondRegistrationsTraceCovers
  • Off_OnceTheOtherRegistrationTurnsItsTraceOff_TheNextPassDrops
  • TheStateKey_TellsWhereTheScopeEndsAndTheExclusionsBegin
  • TheViewerBanner_OnAzureSqlDatabase_SaysEachMonitoredDatabase_NotThisServer
  • TheReadme_SaysTheSessionIsPerDatabase_OnAzureSqlDatabase

Guards, which pass at both commits:

  • ReadOnlyRegistration_Off_DropsItsDatabase_WhenTheLogicalServerExcludesIt (each app): the coverage is exact, so the read-only registration still drops a database that the logical server excludes.
  • On_TheOutsideDrop_StillDropsADatabase_ThatBothLogicalServerRegistrationsExclude (each app): a second logical server registration does not cover every database.
  • DatabaseRegistration_Off_DropsItsDatabase_WhileTheLogicalServerLeavesItToIt (each app): the list of separately monitored databases is the logical server's view. The Darling test builds it with the worker's own function.
  • On_ADatabaseMonitoredAsItsOwnServer_AndScopedOut_IsNotDropped (Darling) and On_ADatabaseMonitoredAsItsOwnServer_AndExcluded_IsNotDropped (Lite): the other registration's trace is off, so only the plan's own-server rule keeps that database. With CanTouch(database) && deleted from the drop outside the set in LongQueryTraceDatabases.Plan, both tests fail. With it restored, both pass.
  • TheRegistrations_AreTheLiveOnesOfTheSameLogicalServer_WithTheirTraceSettingAndScope (Darling): the worker's registration function.
  • TheSweep_PassesTheRegistrationsAndTheServersOwnList_FromTheLiveRegistry (Darling): a source pin on how the sweep passes them, because the sweep needs a live registry.

Existing tests that changed:

  • PostgresEngineGateBehaviorTests (Darling): the reconcile now takes the registrations and the logical server's list. The gate tests pass empty ones.
  • SwallowedItemFailureTests (Darling): its pin on the create loop's text includes the new keptElsewhere argument.
  • LongQueryBannerSessionScopeTests (both apps): the new banner and README sentence.

Round 1

The round 1 tests call test hooks that do not exist on dev, so they cannot compile there. Commit 2e98575 adds the hooks and the tests with no change in behaviour. At that commit, 8 of 9 Lite tests and 9 of 10 Darling tests failed, each on the behaviour that it pins. Commit 821b82e is the fix. The hooks are null in production.

Lite, LongQueryTraceLifecycleLiteTests, RED at 2e98575:

  • Off_DropsTheSessionEverywhere_ExcludedDatabasesIncluded: Assert.Equal() Failure: Collections differ
  • Off_AFailedListing_IsNotMarkedDone_AndTheNextCycleListsAgain: Assert.Null() Failure: Value of type 'Nullable<bool>' has a value
  • Off_OneDatabaseRefusesTheDrop_TheOthersAreStillDropped_AndTheNextCycleRetries: Assert.Null() Failure: Value of type 'Nullable<bool>' has a value
  • Off_TheFifthFailedPassInARow_IsMarkedDone_WithOneWarningThatNamesTheDatabase: Assert.Null() Failure: Value of type 'Nullable<bool>' has a value
  • Off_LeavesTheSessionOfADatabaseMonitoredAsItsOwnServer: Assert.Equal() Failure: Collections differ at index 1
  • On_AnExclusionChange_DropsTheSessionFromTheNewlyExcludedDatabase: Assert.Equal() Failure: Collections differ
  • On_DoesNotCreateTheSessionInADatabaseMonitoredAsItsOwnServer: Assert.Equal() Failure: Collections differ at index 1
  • TheLongQueryRead_SkipsTheDatabasesMonitoredAsTheirOwnServers: Assert.Contains() Failure: Sub-string not found

Darling, LongQueryTraceLifecycleTests, RED at 2e98575:

  • Off_DropsTheSessionEverywhere_ExcludedAndOutOfScopeDatabasesIncluded: Assert.Equal() Failure: Collections differ
  • Off_AFailedListing_IsNotMarkedApplied_AndTheNextSweepListsAgain: Assert.Null() Failure: Value of type 'Nullable<bool>' has a value
  • Off_OneDatabaseRefusesTheDrop_TheOthersAreStillDropped_AndTheNextSweepRetries: Assert.Null() Failure: Value of type 'Nullable<bool>' has a value
  • Off_TheFifthFailedPassInARow_IsMarkedApplied_WithOneWarningThatNamesTheDatabase: Assert.Null() Failure: Value of type 'Nullable<bool>' has a value
  • Off_LeavesTheSessionOfADatabaseMonitoredAsItsOwnServer: Assert.Equal() Failure: Collections differ at index 1
  • On_AScopeChange_DropsTheSessionFromTheDatabaseNowOutOfScope: Assert.Equal() Failure: Collections differ
  • On_AFailedListing_IsNotMarkedApplied: Assert.Null() Failure: Value of type 'Nullable<bool>' has a value
  • On_DoesNotCreateTheSessionInADatabaseMonitoredAsItsOwnServer: Assert.Equal() Failure: Collections differ at index 1
  • TheLongQueryRead_SkipsTheDatabasesMonitoredAsTheirOwnServers: the per-database read filters its list after listing it

The guard TheAlwaysOnSessions_StillFollowTheInventory, in each app, passed at 2e98575 and passes now. It checks that the always-on create step still lists every inventoried database with no scope.

Existing tests that changed in round 1:

  • SwallowedItemFailureTests (Darling): the create loop now runs over the plan's create list, and the drops go through LongQueryTraceDatabases.DropEachAsync. The test follows the new code. It still checks that only the create side is scored.
  • CollectorDatabaseScopeRungTests (Darling): DarlingXeSessions.cs has one unscoped listing fewer, because the trace's on-side listing now passes the scope. The test also checks that the lifecycle gets the scope through DatabaseScopeFor.
  • LongQueryBannerSessionScopeTests (both apps): the new banner and README text.
  • TsqlConventionGuardTests (Darling): its member scan stops short in the two new ListLongQueryTraceDatabasesAsync members, in Lite and in Darling. Each is an expression-bodied member that opens with a property pattern, the shape that the list already carries for Major store upgrade drops operator settings: postgresql.auto.conf is not carried to the new cluster #4253. Two method calls and no string literal fall outside the range. Both members are added to KnownTruncatedRanges.
  • RepoFileAdoptionTests (Darling): the new Darling test file reads a repo file through the LF reader, because one anchor spans a line break. The file is added to the declared list.

Last round: hourly passes that throw, the edition read, the verb's search, the exclusion clause

Seen means the code or a test showed it. Inferred means it follows from the code and no test ran it.

  1. A latched Darling pass that throws no longer repeats on every sweep. Seen: before, a create-only hourly pass that threw left LongQueryTraceAppliedAtUtc an hour old, so every later sweep ran another pass and logged another Warning. An attempt after the cap that threw something other than a failed drop never called RecordFailure, so RetryDue stayed true. Now the create-only pass sets the clock. The attempt after the cap calls RecordFailure and, while the trace is on, sets the clock too. The second part goes one step past the plain fix: without it, the sweep after a throwing attempt runs a create-only pass, because that clock is still an hour old. A full pass keeps its retry on each sweep. The session-missing fault stays set until a pass succeeds (seen in the first new test).
    • Lite: seen in the code, Lite creates the session on every cycle, so a failing create logs a Warning every cycle. Seen in dev's file: origin/dev does the same, with the same catch and the same per-cycle create. Lite's twin needs no change. Inferred: the drop half of a pass after the cap throws only LongQueryTraceDropException (the listing and each drop are wrapped), so a non-drop exception there can only come from the create, which runs every cycle anyway. RetryDue staying true then only keeps the due attempt for when the create works again. No test pins this.
  2. Lite reads the engine edition once per reconcile. Seen: the reconcile read it at the top, in the create and in the drop. A blank edition at the top and 5 at the drop sent the drop down the Azure branch with no registrations and an empty kept list, so it dropped the session from every listed database. RemoteCollectorService now has its own KnownEngineEditions, resolves the edition once, and passes the answer to the create and the drop. Those two were the only callers. A blank status now falls back to the edition this app last saw for the server, so the first reconcile after a start with a blank status is judged as before.
    • Darling: seen in the code, every read in these paths uses Runtime.Target.IsAzureSqlDb (the worker's reconcile and registrations, the session reconcile, the runner's separately-monitored delegate, and the verb's target). IsAzureSqlDb is init-only on CollectorTargetInfo, so it cannot change under one runtime.
  3. --drop-xe-sessions applies the exclusions to the long-query session. Seen: the verb listed databases with the server's exclusions, so it never searched an excluded database, and the long-query session stayed there. Now, for that session only, it searches every online database, the exclusions not applied, as the off-side reconcile does. It skips master, a database monitored as its own server, and a database where another registration of the logical server keeps the session. It reuses the worker's rule (LongQueryTraceRegistrations, LongQueryTraceServerSeparatelyMonitored, LongQueryTraceDatabases.Plan and KeptElsewhere). The deadlock and blocked-process sessions keep today's list. The verb gets the registry's servers but cannot read another registration's long-query schedule, so it counts every other registration of the logical server as keeping the session. Inferred: when two registrations of one logical server both have the trace off, each run of the verb leaves the session in the databases the other covers. The two doc comments, the README and the verb's usage text now name the sets. It does a second listing of master only when the server has exclusions.
    • Lite: seen, Lite has no equivalent cleanup command (no such verb or class in the Lite sources), so nothing changes there.
  4. The exclusion clause has a pin in each app. Each lifecycle test replaces the listing, so nothing pinned the applyExclusions ternary. The ternary now sits in its own small member (DarlingCollectorRunner.AzureDatabaseListPlan, RemoteCollectorService.BuildAzureDatabaseListQuery), with no change in behavior. The new test in each app checks that with applyExclusions false and excluded databases set, the query has no NOT IN clause and no parameters, and with it true, it has the clause and two parameters.
  5. LongQueryTraceDropRetry.ConsecutiveFailures is removed. Seen: nothing in the product read it. One Darling test asserted it, and that line is gone. The test still asserts the next attempt is cleared and nothing runs an hour later.

An excluded database the verb cannot open

  1. Excluded databases the verb cannot open are notes now.
    Seen: the long-query search opens the databases the server excludes. Before, a database that refused the connection set exit code 2, even when every session found was dropped. Before this PR the verb never opened an excluded database. A login with no user there, or a paused serverless database, then stopped a script that removes the server next. Now AzureSearchPlan.Unsearched files a monitored database it cannot open as a problem, with exit 2 as before. It files an excluded one as a note on stderr, and the exit code stays the same. The usage text and the README say so.
  2. The create side's retry cadence is documented.
    The summary of ReconcileLongQueryCompletionsAzureAsync now says why the create side has no cap. It retries on every sweep, so the fault is recorded again and the run reads SESSION_MISSING. A CREATE that every database refuses has the same cadence. Only the drop side is capped. Lite and Darling log a Warning on every cycle while the long-query session cannot be created #4964 tracks the repeated Warnings this logs.

RED, with the note's record and seam in place and the old behavior (not a separate commit), DropXeSessionsVerbTests 4 failed of 92:

  • AnExcludedDatabaseTheLoginCannotOpen_IsANoteOnStderr_AndTheRunStillSucceeds: Expected: 0, Actual: 2
  • ADatabaseSearchedOnlyForTheLongQuerySession_IsFiledAsANote_AndAMonitoredOneAsAProblem: Assert.False() Failure
  • ANoteFromTheSearch_IsPrintedOnStderr_AndLeavesTheExitCodeAndTheDropsAlone: expected the note, got an empty string
  • ANoteWithNothingFound_StillSaysNothingWasDroppedOnlyInThePlacesSearched: Sub-string not found

The guard AMonitoredDatabaseTheLoginCannotOpen_StillFailsTheRun_WithTheUnavailableCode passes before and after.

RED at f07fc9e (the tests-first commit, with the seams in place and the old behavior):

  • Darling LongQueryTraceLifecycleTests.On_AnHourlyCreatePassThatThrows_RunsAgainAnHourLater_NotOnEverySweep: Assert.Equal() Failure: Values differ
  • Darling LongQueryTraceLifecycleTests.On_TheAttemptAfterTheCap_ThatThrowsAnotherError_RunsAgainAnHourLater_NotOnEverySweep: Assert.Equal() Failure: Values differ
  • Lite LongQueryTraceLifecycleLiteTests.Off_AnEditionThatChangesDuringTheReconcile_StillLeavesTheDatabasesAnotherRegistrationKeeps: Assert.Empty() Failure: Collection was not empty
  • Darling DropXeSessionsVerbTests.TheLongQuerySearch_VisitsAnExcludedDatabase_AndSkipsOneMonitoredAsItsOwnServer: Assert.Equal() Failure: Collections differ at index 0
  • Darling DropXeSessionsVerbTests.TheLongQuerySearch_SkipsADatabaseAnotherRegistrationOfTheServerKeeps: Assert.Equal() Failure: Collections differ at index 0
  • Darling DropXeSessionsVerbTests.ATargetWithItsOwnNames_SearchesNoDatabaseForTheLongQuerySession: Assert.Empty() Failure: Collection was not empty

The two exclusion-clause tests describe behavior the code already had, so they pass at that commit. Proof that they pin it: with each ternary inverted, TargetProviderTests.TheRunnersAzureDatabaseList_AppliesTheServersExclusionsOnlyWhenAsked (Darling) and LongQueryTraceLifecycleLiteTests.TheAzureDatabaseList_AppliesTheServersExclusionsOnlyWhenAsked (Lite) both fail with Assert.DoesNotContain() Failure: Sub-string found. Both ternaries are restored.

At cf5bd03 with 0 build warnings in both projects, these classes pass: Darling LongQueryTraceLifecycleTests 28 total, 0 failed; DropXeSessionsVerbTests 87 total, 0 failed; TargetProviderTests 30 total, 0 failed; PostgresEngineGateBehaviorTests 8 total, 0 failed, 4 skipped. Lite LongQueryTraceLifecycleLiteTests 23 total, 0 failed; LongQueryBannerSessionScopeTests 3 total, 0 failed. Those runs came before the README and usage-text edit of the verb. That edit is covered only by the full run below.

Existing tests that changed in this round: DropXeSessionsVerbTests (the injected connector now also receives the registry's servers).

Full suites

At 5712458 (dev merged in), with 0 build warnings in both projects:

  • Lite.Tests: 7201 total, 0 failed, 0 skipped.
  • Darling.Tests without PostgreSQL: 19938 total, 0 failed, 1320 skipped, 1 not run.
  • Targeted at this head: DropXeSessionsVerbTests, DropXeSessionsHelpGuidanceTests, DropXeSessionsLiveTests, LongQueryTraceLifecycleTests, TsqlConventionGuardTests and DocCommentHygieneTests, 227 run, 0 failed, 1 skipped (a live test that needs a SQL Server).
  • The PostgreSQL-backed tests run in CI. At cf5bd03, CI passed the build, every Lite test shard and every Darling PostgreSQL shard.

CHANGELOG

No entry in this PR; the release owner adds it under [Unreleased].

Proposed entry (changed in the last round, because the verb now does something new for a user):

On Azure SQL Database, the long-query trace runs only in monitored databases, and turning it off no longer removes a trace that another registration of the same database still uses. Darling now re-creates a long-query trace session dropped from outside within an hour, instead of reading nothing until it reconnects, and waits an hour before it tries again after a failed hourly attempt. --drop-xe-sessions now also drops the long-query trace session from the databases a server excludes, and still leaves any database that is monitored as its own server or where another registration keeps the session. An excluded database it cannot open is reported in a note and does not change the exit code. Lite no longer drops the session from a database another registration keeps when the engine edition reads blank and then 5 during one cycle.

…abase, in Lite and the Darling Viewer

While the long-query completion trace is off, both apps show a banner that ended with "Enabling it creates the
Extended Events session on this server". On Azure SQL Database the collector creates a database-scoped session in
each monitored database instead, so the banner was wrong for that target.

The banner text now comes from the target. LongQueryCompletionsCollector.SessionScopeSentence gives the last
sentence for both apps: each monitored database on Azure SQL Database, this server everywhere else. Lite reads the
target from the tab's engine edition, which it resolves before every tab load. The Viewer reads it from the
server's stored engine edition.

The Darling README and the CollectorScheduleDefaults comment for the collector now say the same.
Comment thread Darling/PerformanceMonitor.Darling.Viewer/ViewerServerTab.LongQueries.cs Outdated
Comment thread Lite/Controls/ServerTab.LongQueries.cs Outdated
@claude

claude Bot commented Oct 2, 2026

Copy link
Copy Markdown

Blind read first, then the body. The mechanism holds end to end: I traced the claim the banner now makes back to the code that performs it in both SKUs, not just to the sentence that describes it.

  • Lite — RemoteCollectorService.LongQueryCompletions.cs: EnsureLongQueryCompletionsXeSessionAsync and DropLongQueryCompletionsXeSessionAsync both branch on engineEdition == 5 into EnsureDatabaseScopedXeSessionsAsync / a per-database drop loop (master skipped). So "creates … in each monitored database / drops … from each one" is what Lite actually does.
  • Darling — DarlingXeSessions.ReconcileLongQueryCompletionsAsync → ReconcileLongQueryCompletionsAzureAsync on server.Target.IsAzureSqlDb, enumerating via GetAzureDatabaseListAsync(server, databaseScope: null, …) so the inventory stays server-shaped, master skipped, enable and disable on the same loop. Same sentence, same truth.
  • LongQueryCompletionsCollector.RunsPerDatabase => target.IsAzureSqlDb, and CollectorEngineCapability.IsAzureSqlDb = engineEdition == AzureSqlDatabaseEngineEdition (= 5), which is the same 5 that ServerHardwareScope.AzureSqlDatabaseEngineEdition and both lifecycle gates use. One number, four spellings, no drift.

I also chased the four ways this shape has bitten before and found none of them here:

  • Surfaces missed. grep "session on this server|session on the server|creates an Extended Events session" across .cs/.xaml/.html/.js/.md returns only the new helper, the two new tests, the CollectorScheduleDefaults comment and the generic docs/how-collection-works.md line. The web viewer's copy really is scope-free — wwwroot/js/pages/server-tabs.js:1324 is "No long-running completions in this window. This collector is opt-in and off by default." — so the "Checked, no change" note checks out.
  • Existing README pins. DarlingCliCommandsTests.Readme_DocumentsBothVerbs_… wants creates the \PerformanceMonitor_LongQueryCompletions` Extended Events sessionanddisabling it drops that session; both survive the insert. DropXeSessionsVerbTests.TheReadmeSaysWhenToRunTheNamedForm_…anchors on"a server removed while the collector was on keeps the session."and reads 500 chars forward forjust before you remove the serverand ``--drop-xe-sessions --print-sqlafterwards `` — the new sentence lands *before* that anchor and the window still ends ~278 chars in. TheIt does that only …→The service does that only … rewording is pinned by nothing (DropXeSessionsHelpGuidanceTests` asserts "still monitors" against a CLI help line, not the README).
  • Dead pin / timing. Lite's _isAzureSqlDatabase is live at RefreshLongQueriesAsync because RefreshVisibleTabAsync awaits RefreshEngineEditionAsync() ahead of its switch, which AzureSqlDatabaseCallSiteTests.TheTab_FallsBackToTheStoredEdition_BeforeEveryLoad already pins. Long Queries has no sub-tab Refresh button bypassing that path (sole caller is ServerTab.Refresh.cs:243), so it does not belong in TheSubTabRefreshButtons_LearnTheEngineEdition_…'s roster.
  • Empty banner. Dropping Text from both XAMLs is safe only because the TextBlock ships Visibility="Collapsed" and the only writer of its Visibility is the same method that now writes its Text first — confirmed, one site per SKU.

Three non-blocking notes, none of which I think should hold the PR:

  1. The new README pin runs on no README-only PR. Darling.Tests.TheReadme_SaysTheSessionIsPerDatabase_OnAzureSqlDatabase reads Darling/README.md, and build.yml's darling filter is Darling/**/!(*.md) while docs is **/*.md — so a PR that reworded that sentence back takes the docs fast path and never runs the guard. That is the documented trade (build.yml lines 140-147, Erik's 2026-09-24 ruling, with LiteRuntimePrerequisiteDocsTests as the precedent: README guards earn their keep on every code PR plus the nightly), so I am flagging it as a known property of the pin rather than a defect. It is not a cross-app read, so CrossAppGuardCiGateTests has nothing to say about it either.
  2. The two SKUs' helpers took different signatures — ServerTab.LongQueriesDisabledText(bool isAzureSqlDatabase) vs ViewerServerTab.LongQueriesDisabledText(int engineEdition) — which is fine (each takes what its host already holds) but means the two parity tests cannot be written against one shape. Related: Darling.Tests' BannerElement is internal static as if to be shared, while Lite.Tests open-codes the same four lines inline. Either share it or make it private.
  3. The two SKUs resolve the edition differently, and the body says so for one of them. Lite falls back to the newest collected server_properties row when the connection check returned 0; Darling reads servers.sql_engine_edition straight off _server.EngineEdition with no fallback, so an Azure SQL Database the service has never successfully probed gets the server sentence. The body states this ("An unknown edition gets the server sentence") and it matches every other engine-aware Viewer surface — ViewerAzureSqlDatabaseCallSiteTests.TheMemoryOverview_ReadsTheRegistrysEdition_… pins the registry as the rule, and DarlingObservability's registration upsert stamps that column from the connect probe — so it is consistent, not a gap.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: LGTM

Claims

  • VERIFIED — the old banner ended "…session on this server; disabling it drops the session." — the removed Text attributes, Lite/Controls/ServerTab.xaml:3120 and ViewerServerTab.xaml:3751 (LEFT side).
  • VERIFIED — "wrong on Azure SQL Database… creates a database-scoped session in each monitored database (RunsPerDatabase)" — LongQueryCompletionsCollector.cs:135, and both lifecycles actually do it: Lite/Services/RemoteCollectorService.LongQueryCompletions.cs:115 and :213 (enable and drop, per database, master skipped), DarlingXeSessions.cs:583 into :648.
  • VERIFIED — "SessionScopeSentence(bool) holds the last sentence. Both apps use it." — LongQueryCompletionsCollector.cs:143-145, called at ServerTab.LongQueries.cs:29 and ViewerServerTab.LongQueries.cs:26; literals byte-identical to the asserted ones (grep -c is 1 in all four files).
  • VERIFIED — "Lite: the tab resolves the edition before every tab load" — ServerTab.Refresh.cs:214 awaits RefreshEngineEditionAsync() ahead of the switch reaching case 20 at :243; pinned by AzureSqlDatabaseCallSiteTests.TheTab_FallsBackToTheStoredEdition_BeforeEveryLoad.
  • VERIFIED — "Darling Viewer: the server's stored engine edition. An unknown edition gets the server sentence." — ViewerServerTab.LongQueries.cs:39 reads _server.EngineEdition (servers.sql_engine_edition, ViewerDataService.cs:96); TheViewerBanner_OnEveryOtherEdition_KeepsTheServerSentence carries InlineData(UnknownEngineEdition).
  • VERIFIED — "Both XAML files drop the fixed Text. The refresh sets it instead." — safely: Visibility="Collapsed" stays in markup and the sole writer of that Visibility per SKU is the same method that now writes Text first.
  • VERIFIED — "Darling README gets one sentence for Azure SQL Database" — Darling/README.md:741, string-identical to the new pin.
  • VERIFIED — "The CollectorScheduleDefaults comment says the same." — CollectorScheduleDefaults.cs:77.
  • VERIFIED — "the web viewer's text says nothing about where the session lives" — wwwroot/js/pages/server-tabs.js:1324 is only "This collector is opt-in and off by default."; a repo-wide grep for the server-scope phrasings finds no other claiming surface.
  • VERIFIED — "existing README pins in DarlingCliCommandsTests and DropXeSessionsVerbTests still pass" — DarlingCliCommandsTests.cs:2124-2125 wants substrings the edit leaves intact; DropXeSessionsVerbTests.cs:809-812 anchors on "a server removed while the collector was on keeps the session." and the new sentence is inserted before that anchor, with both required substrings still ~228 and ~278 chars into its 500-char window.
  • VERIFIED — "the old words, split into two sentences" — semicolon to period only, LongQueryCompletionsCollector.cs:145.
  • UNVERIFIED — "RED on dev, with the failing lines" (three named assertions) — needs the tests checked out against the pre-change tree and run; two cannot compile there by the body's own admission.
  • UNVERIFIED — "Lite.Tests 7160 / Darling.Tests 19878, 0 failed" — net10.0-windows WPF suites; this review ran on Linux with no build.

Riskiest lines

  1. Darling/PerformanceMonitor.Darling.Viewer/ViewerServerTab.LongQueries.cs:39 — edition source with no fallback: a registry sql_engine_edition of 0 on a real Azure SQL Database prints the server sentence. Call site pinned by TheViewerBanner_TakesItsTextFromTheTarget_NotFromAFixedServerSentence, unknown-edition answer by TheViewerBanner_OnEveryOtherEdition_KeepsTheServerSentence; UNPINNED for whether the column is populated (rests on DarlingObservability.cs:40's connect-probe upsert, untested here).
  2. Lite/Controls/ServerTab.LongQueries.cs:40 — reads _isAzureSqlDatabase at loader time, so correctness lives in another file's ordering. Pinned by AzureSqlDatabaseCallSiteTests.TheTab_FallsBackToTheStoredEdition_BeforeEveryLoad plus TheLiteBanner_TakesItsTextFromTheTarget_NotFromAFixedServerSentence.
  3. Lite/Controls/ServerTab.xaml:3120 and ViewerServerTab.xaml:3751 — the banner has no markup text now, so any path showing it without the loader renders empty. Pinned negatively by the two _TakesItsTextFromTheTarget_ facts and by there being one Visibility writer per SKU; UNPINNED for "the XAML carries no Text at all", so a re-added literal passes both.

Traps near this diff

  • "Lite and Darling ship in parity (CONTRIBUTING's two-store rule): a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply — both halves ship together with their own tests: ServerTab.LongQueries.cs:25 with Lite.Tests/LongQueryBannerSessionScopeTests.cs, ViewerServerTab.LongQueries.cs:23 with Darling/Darling.Tests/LongQueryBannerSessionScopeTests.cs.
  • "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — does not apply — the shared half is aliased: one SessionScopeSentence at LongQueryCompletionsCollector.cs:143 used by both SKUs; only the menu path stays per-SKU, and it genuinely differs between the two apps.
  • "A roster-based census does not see the file it does not roster (#3699)" — does not apply — the nearest roster, AzureSqlDatabaseNotCollectedSurfaceTests.TheSubTabRefreshButtons_LearnTheEngineEdition_BeforeTheirLoaderRuns, covers handlers that bypass RefreshVisibleTabAsync; RefreshLongQueriesAsync's only caller is ServerTab.Refresh.cs:243, inside it, so there is no bypass to roster.
  • "InternalsVisibleTo on PerformanceMonitor.Analysis names Lite.Tests only — a helper Darling.Tests must call has to be public (#3688)" — does not apply — PerformanceMonitorLite.csproj:44 grants Lite.Tests and PerformanceMonitor.Darling.Viewer.csproj:66 grants Darling.Tests; Lite.Tests' using Darling.Tests resolves to the linked CSharpSourceWalker.cs (Lite.Tests.csproj:40), not a project reference.
  • "dorny/paths-filter v4 evaluates each pattern as an INDEPENDENT predicate … Keep the carve-out INSIDE the include: dir/**/!(*.md)" — does not apply as a defect, but it is why the new README pin cannot fire on a README-only PR: build.yml:181 is Darling/**/!(*.md) while docs is **/*.md. build.yml:140-147 records that as Erik's 2026-09-24 ruling with LiteRuntimePrerequisiteDocsTests as the accepted precedent.
  • "An expression-bodied member holding a { } property pattern is read SHORT by the T-SQL convention guard's member walk (#3667)" — does not apply — both new members hold only string concatenation and one equality comparison (ServerTab.LongQueries.cs:25-29, ViewerServerTab.LongQueries.cs:23-26).
  • ".gitattributes says text=auto eol=crlf: a diff that touches every line of a file is a line-ending conversion until proven otherwise" — does not apply — all five touched or added .cs files are UTF-8 CRLF and every hunk is local.

Divergence (blind read vs PR body)

One, cosmetic. The body describes the loader change as setting Text; the diff also clips the space after the equals sign on the one pre-existing line it touches in each SKU — the Visibility assignment at ViewerServerTab.LongQueries.cs:40 and ServerTab.LongQueries.cs:41. Nothing else: every mechanism the body names is implemented, and every surface the diff touches is accounted for. Unmentioned but not charged: the helpers took different signatures (bool in Lite, int in Darling), and Darling.Tests' BannerElement is internal while Lite.Tests open-codes the same logic inline.

Reviewer checklist

  • Run both suites on Windows and confirm the stated counts; nothing was built here, so the new using PerformanceMonitor.Collectors; in two WPF partials and the internal visibility of both helpers are unverified compiles.
  • Check the two test files out against dev and confirm the three named assertions go RED for the stated reasons.
  • On a live Azure SQL Database in Darling, open Long Queries with the trace off and confirm servers.sql_engine_edition is 5 so the banner reads "in each monitored database"; then the same in Lite against a target whose connection check failed, so the stored-edition fallback answers.
  • Eyeball both banners: the markup carries no Text now, so confirm no path shows an empty warning block and that the arrow and ellipsis render as glyphs, not mojibake, from BOM-less UTF-8 C#.

…they drive

No behaviour change. The new tests fail here on purpose: they describe the
fix in the next commit.

- A shared helper in Collectors plans where the trace's database-scoped
  session is created and dropped, counts failed drop passes, and carries the
  databases a drop failed in. Nothing calls it yet.
- Lite and Darling list the trace's databases and work in one database through
  hooks a test can replace. Darling's worker bookkeeping for the trace is a
  static method the tests call, and the runner takes the databases monitored
  as their own servers.
…s, retries a failed drop, and leaves separately monitored databases alone

In both Lite and Darling, for the database-scoped session on Azure SQL Database:

- Trace on: the session is created in each monitored database (Darling: narrowed
  by the collector's database scope, resolved by the same runner delegate as the
  read loop), and dropped from listed databases outside that set.
- Trace off: the session is dropped from every listed database, exclusions and
  scope included.
- A database monitored as its own server is left to that registration: never
  created in, never dropped, and the logical server's long-query read skips it.
- A failed listing or a failed drop is retried on the next cycle instead of
  being counted done. After 5 failed passes in a row it is counted done, with
  one warning that names where the session may remain. A drop failure in one
  database no longer stops the others, and Lite logs it at Warning.
- Darling's ON arm no longer counts the trace applied when the database list
  could not be read.

The always-on deadlock and blocked-process sessions keep their inventory
lifecycle. The banner, README, Lite's schedule description and Darling's CLI
help say where the session is created and dropped.
…inventory, and its lifecycle test as an LF reader

Both listings are expression-bodied members that open with a property pattern, so the member scan stops at the pattern's closing brace. What falls outside the range is two calls and no string literal. The lifecycle test's always-on guard slices a method body on an anchor that spans line breaks.
…ession

Lite's read fails there every cycle. Darling's reads zero rows that look like a quiet trace. The comment said both fail.
…own server follows that server's setting, when enabling too

The exception sat only on the disable sentence. Enabling skips that database as well, so it is now a sentence of its own that covers both.
@erikdarlingdata erikdarlingdata changed the title Long Queries banner says the session is per database on Azure SQL Database, in Lite and the Darling Viewer On Azure SQL Database, the long-query trace runs only in monitored databases and leaves a separately registered database to its own registration Oct 2, 2026
Comment thread Darling/PerformanceMonitor.Darling.Service/DarlingCollectorRunner.cs Outdated
Comment thread Lite/Services/RemoteCollectorService.cs Outdated
erikdarlingdata added a commit that referenced this pull request Oct 2, 2026
The Opus tier capped at 60 turns, and large PRs need 60-70: #4946's review hit the cap twice at its final head with no verdict, and #4944 and #4945 finished at 69. The light tier stays at 30.
…database another registration still uses

Adds the registrations each app passes to the shared plan, KeptElsewhere, the co-owner part of the state key, and the tests that pin the rule in Lite and Darling. KeptElsewhere and the co-owners still count no other registration here, and the state key still joins its parts with the names' separator, so the behavior is as before and the new tests show it.
…e another registration of the server has the trace on

KeptElsewhere names the databases where another enabled registration of the same logical server, of any read-only intent, keeps the session: a registration of one database keeps its own, and a logical server's registration keeps each database it creates the session in. Both drops in both apps skip them. The state key carries those registrations, so a skipped drop runs once the other one turns its trace off, and its parts now use their own separator, so a scope and an exclusion list cannot read two ways. The banner and the Darling README say so.
@erikdarlingdata erikdarlingdata changed the title On Azure SQL Database, the long-query trace runs only in monitored databases and leaves a separately registered database to its own registration On Azure SQL Database, the long-query trace runs only in monitored databases and no longer drops a session another registration still uses Oct 2, 2026
Comment thread Lite/Services/RemoteCollectorService.LongQueryCompletions.cs Outdated
Comment thread PerformanceMonitor.Collectors/LongQueryTraceDatabases.cs Outdated
@claude

claude Bot commented Oct 2, 2026

Copy link
Copy Markdown

Blind-read notes, all non-blocking

1. The drop pass now pays a connection per excluded / out-of-scope database, and each failure is a Warning.

On the ON path the drop list is listed − monitored − master − separatelyMonitored − keptElsewhere (LongQueryTraceDatabases.cs:184-189), and listed comes from the new applyExclusions: false enumeration. On a logical server where a database is excluded because the login has no access to it — a common reason to exclude — OpenAzureDatabaseConnectionAsync fails there on every pass, so one state-key change or restart costs 5 passes × N failed logins plus the give-up Warning. Lite's move from Debug to Warn (RemoteCollectorService.LongQueryCompletions.cs:433) makes that newly visible in the log, and Darling re-arms it on every reconnect (DarlingWorker.cs:10462, LongQueryTraceDropRetry.Reset()), so a flapping server re-enters the loop rather than staying given-up. The DropAttemptCap of 5 is what bounds it and the body documents the reasoning, so this is an observation about the volume, not a request — but it is the one cost of the new rule that no test measures.

2. The read-skip has no behavioural pin in either SKU.

SkipsSeparatelyMonitoredDatabases changes which databases the collector reads, which is a capture-coverage change (DarlingCollectorRunner.cs:2237, RemoteCollectorService.DefinitionRunner.cs:285). Both TheLongQueryRead_SkipsTheDatabasesMonitoredAsTheirOwnServers tests are source-text pins — IndexOf ordering plus Assert.Contains on the call site — and the Lite one says so. The lifecycle half gets a real rig; the read half would fail only on the line being deleted, not on the filter being applied to the wrong list or against the wrong separatelyMonitored set. The Lite read's set comes from SeparatelyMonitoredDatabasesFor with isAzureSqlDatabase: true hardcoded (RemoteCollectorService.LongQueryCompletions.cs:402), a third derivation of the Azure verdict beside the two in the inline comment on :80; it is sound today only because RunsPerDatabase already gated on target.IsAzureSqlDb.

3. LongQueryTraceRegistrationsFor keys on ServerConnection.DatabaseName, while the database list keys on the connection string's Initial Catalog.

RemoteCollectorService.LongQueryCompletions.cs:521 reads other.DatabaseName to decide logical-server-vs-one-database, but GetAzureDatabaseListAsync decides the same thing from new SqlConnectionStringBuilder(baseConnStr).InitialCatalog (RemoteCollectorService.cs:1064). They agree for every registration whose resolver copies DatabaseName into the catalog — which is the shape #2228 exists because they can diverge on. Worth one line of comment saying why the two readings cannot disagree here, or routing both through the same accessor. Darling has the same split (DarlingWorker.cs:4087 reads other.Database, DarlingCollectorRunner.cs:6593 reads the catalog).

4. CHANGELOG. [Unreleased] is empty and this is a user-visible behaviour change, but the body says so explicitly and hands the entry to the release owner with proposed prose, so it is a stated handoff rather than an omission.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: LGTM

Detail is in the two inline comments and the top-level note; this is the ledger.

Claims

  • VERIFIED — the rule lives once in LongQueryTraceDatabases and both apps plan through it — Plan is the only create/drop planner on either side: DarlingXeSessions.cs:692/:728/:792, RemoteCollectorService.LongQueryCompletions.cs:196/:319/:568
  • VERIFIED — off drops from every listed database, exclusions and scope not applied, master never touched — applyExclusions: false + databaseScope: null (DarlingCollectorRunner.cs:6634, RemoteCollectorService.cs:1116); CanTouch and the off arm at LongQueryTraceDatabases.cs:172-181
  • VERIFIED — Darling's lifecycle scope is the delegate the read loop resolves — DatabaseScopeFor returns _databaseScope(...) (DarlingCollectorRunner.cs:400), resolved at dispatch at :1978
  • VERIFIED — a database monitored as its own server is neither managed nor read by the logical server's registration, for this collector only — LongQueryTraceDatabases.cs:170, DarlingCollectorRunner.cs:2237, RemoteCollectorService.DefinitionRunner.cs:285; the one override => true is LongQueryCompletionsCollector.cs:144
  • VERIFIED — KeptElsewhere ignores read-only intent, SeparatelyMonitoredDatabases honours it, coverage is exact — LongQueryTraceDatabases.cs:236-243 and :252 vs AzureMasterScope.cs:49, pinned by On_TheOutsideDrop_StillDropsADatabase_ThatBothLogicalServerRegistrationsExclude in both SKUs
  • VERIFIED — the state key's parts, its four separators, the 5-pass cap and its resets — LongQueryTraceDatabases.cs:117/:122-125/:203-207/:337-350; resets at RemoteCollectorService.LongQueryCompletions.cs:361, DarlingWorker.cs:4155/:10462
  • VERIFIED — the dev defect named: a failed listing while enabling latched "applied" — return null; becomes throw; at DarlingXeSessions.cs:716, and that return is what let the worker set LongQueryTraceApplied = enabled
  • VERIFIED — Lite warns rather than debugs each failed drop (RemoteCollectorService.LongQueryCompletions.cs:433 vs DarlingXeSessions.cs:837); the registry is published before the runner is built (DarlingWorker.cs:2619 vs :2649); both banners read the target's edition with the space after = restored (ServerTab.LongQueries.cs:40, ViewerServerTab.LongQueries.cs:39) and the sentence pair exists only at LongQueryCompletionsCollector.cs:151-153
  • VERIFIED — the stated mutation and the changed pins — without CanTouch(database) && the ON drop admits master and the own-server database, failing both On_ADatabaseMonitoredAsItsOwnServer_... tests; one unscoped listing remains (DarlingXeSessions.cs:121); the SwallowedItemFailureTests text pins match :728-731/:761-764/:787/:800/:824 byte for byte
  • UNVERIFIED — the run counts (Lite 21/0 and Darling 64/0/4 at 464e314; Lite 7169/0 and Darling 19888/0/1320/1 at 7cc63c1) and "0 build warnings" — needs a build and test run; dotnet build was unavailable here and Lite/Viewer are Windows-only WPF
  • UNVERIFIED — every RED-at-2e98575db / RED-at-d1a84993e assertion, and that the pre-fix single-separator StateKey collapsed two settings onto one key — intermediate commits; only HEAD is readable, where all are green by construction. A single separator does collapse both strings, so the mechanism is sound
  • UNVERIFIED — the Microsoft Learn read-only-replica facts and the one the body labels "inferred" (a primary drop removes the session from the replicas) — external documentation

Riskiest lines

  1. PerformanceMonitor.Collectors/LongQueryTraceDatabases.cs:189 — the ON-path drop set; a wrong ownedElsewhere or kept removes a session another registration is still reading, the defect class this PR closes — pinned by On_TheOutsideDrop_LeavesAnExcludedDatabase_ThatASecondRegistrationsTraceCovers and On_TheOutsideDrop_StillDropsADatabase_ThatBothLogicalServerRegistrationsExclude in both lifecycle suites, which drive the real reconcile
  2. Lite/Services/RemoteCollectorService.LongQueryCompletions.cs:80 — the Azure verdict every protective set hangs off, re-derived at :181 and :314 from a second read of the same mutable SqlEngineEdition; a disagreement runs the OFF drop with empty separatelyMonitored and empty keptElsewhere — UNPINNED
  3. DarlingCollectorRunner.cs:2239 / RemoteCollectorService.DefinitionRunner.cs:287 — the read-skip, a capture-coverage change: an own-server database with its own trace off now has no capture at all — pinned only by the source-text TheLongQueryRead_SkipsTheDatabasesMonitoredAsTheirOwnServers, which fails on the line's deletion but not on the filter being fed the wrong set

Traps near this diff

  • "Lite and Darling ship in parity (CONTRIBUTING's two-store rule): a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply: plan/key/retry/exception shared in LongQueryTraceDatabases.cs, read filter shared via AzureSweepScope.cs:49 and applied in both runners, banners through LongQueryCompletionsCollector.cs:151, applyExclusions in both (DarlingCollectorRunner.cs:6591, RemoteCollectorService.cs:1060), Lite's lack of a scope explicit at RemoteCollectorService.LongQueryCompletions.cs:528
  • "A default interface member compiles a parity gap away ... CONTRIBUTING's two-store parity rule names IAlertDeliverer; its members are required." (#3640) — does not apply: the new default is on ICollectorDefinition<TRow> (ICollectorDefinition.cs:54), not a per-store seam; that interface already carries defaults of this shape (:118) and every collector reaches it through one CollectorDefinitionBase.cs:84
  • "An expression-bodied member holding a { } property pattern is read SHORT by the T-SQL convention guard's member walk (#3667)" — applies, handled as the roster already does: both new ListLongQueryTraceDatabasesAsync members have that shape, are declared with their reason at TsqlConventionGuardTests.cs:739/:749, and the stranded arms hold no string literal
  • "A pin that encodes the lie ... Ask what a pin would fail ON" and "A roster-based census does not see the file it does not roster (#3699)" — do not apply as gaps: the lifecycle tests drive the real reconcile, the two guard tests declare themselves source-text pins in their own summaries, and the new file is rostered at RepoFileAdoptionTests.cs:679
  • "OPTION(RECOMPILE) on collector queries" and "missing-index requests are never a finding's driver", plus the Collectors/Analysis/Alerting/Storage trap groups — do not apply: no T-SQL statement is added (only the exclusion clause becomes conditional, SqlServerTargetProvider.cs:142 and RemoteCollectorService.cs:1116; the DDL from DarlingXeSessions.cs:842 on is unchanged), no index recommendation, no delta family, rate column, fact, scorer, alert condition or paged payload

Divergence (blind read vs PR body)

None material. Three readings narrower or wider than the body's, none contradicting it:

  • "If the database listing fails, the reconcile throws" holds, but Lite's ON-side catch is catch (SqlException ex) alone (RemoteCollectorService.LongQueryCompletions.cs:190), so a non-SqlException failure reaches the general handler raw. Same outcome, so the claim stands; the wrapping is just not universal.
  • "On, drop outside the set: No" holds for every case except the one the body then discloses as "One limit remains" — consistent, but stated in two separate sections.
  • The body does not mention the ON-path drop pass's cost against excluded-and-inaccessible databases, nor the third derivation of the Azure verdict at RemoteCollectorService.LongQueryCompletions.cs:402. Both are in my top-level comment.

Reviewer checklist

  • Build and run both suites: every count and RED/GREEN-at-commit claim is unverified here. Confirm Lite.Tests still compiles Darling.Tests' CSharpSourceWalker, that the new static/instance KnownEngineEditions.SeparatelyMonitoredDatabases overloads resolve without ambiguity, and that DarlingSelfAlertTests.CapturingLogger is reachable from LongQueryTraceLifecycleTests.
  • On a live Azure SQL Database logical server with one database excluded and the login denied access to it, turn the trace on: expect 5 failed logins and exactly one "Stopped retrying" Warning, and check that reconnect re-arming the counter (DarlingWorker.cs:10462) is not a per-reconnect loop on a flapping server.
  • With a second registration of one database on that server (read-only-intent and read-write), toggle each trace and confirm from sys.database_event_sessions that no drop removes the other registration's session, and that the read-skip leaves no database both unread and session-bearing.
  • Open Long Queries in Lite and the Viewer the instant an Azure target connects: the banner's last sentence must be the per-database one, now that it comes from the edition at refresh time rather than static XAML.

…p, and for Darling creating a session dropped from outside
…Debug; Darling creates a session dropped from outside within an hour
…eates a missing long-query session again within an hour
Comment thread Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs
Comment thread Lite/Services/RemoteCollectorService.LongQueryCompletions.cs Outdated
@claude

claude Bot commented Oct 2, 2026

Copy link
Copy Markdown

Non-blocking: the ownership rule creates a coverage hole the UI does not announce

Flagging as an observation, not a finding — the code documents it as deliberate (LongQueryCompletionsCollector.cs, the SkipsSeparatelyMonitoredDatabases comment) and the PR body states it — but it is worth a second look because it diverges from how the sibling sessions treat the same databases.

After this change, a logical server's registration neither creates the trace's session in, nor reads, a database monitored as its own server. That database is covered only while its own registration's trace is on, and long_query_completions ships off. So on a logical server where the operator registered a few databases separately and then turned the trace on at the master registration, those databases are silently uncovered: no session, no read, no banner, and the Long Queries grid for the master registration simply has fewer databases in it than the operator asked for.

The always-on pair resolves the same conflict the other way: EnsureDatabaseScopedAsync still provisions deadlock and blocked-process sessions in every inventoried database whatever the ownership, and only the counts skip the separately monitored ones — AzureMasterScope.SeparatelyMonitoredListNote says so in the UI ("Events from databases monitored as their own servers are listed here and counted under those servers"). The long-query trace now skips them at the lifecycle and at the read, which is a stronger rule with no equivalent note.

The reasons given for the stronger rule are sound as far as they go (#4731's read failure, and double-storing while both traces are on). What is missing is the operator-facing half: nothing tells the master registration's reader that some of its databases are deliberately out of its Long Queries scope. The SeparatelyMonitoredListNote precedent is the shape that would close it.

Other claims checked against the tree, for the record

Verified from the tree, as the body states them:

  • LongQueryTraceDatabases.Plan never touches master or a separately monitored database on either side (CanTouch, lines 78-80), and kept is applied to the drops only (lines 87, 92).
  • The state key's four distinct separators (lines 47-50), and TheStateKey_TellsWhereTheScopeEndsAndTheExclusionsBegin asserts the non-collision the body describes.
  • _registryState.Publish(initialServers) at DarlingWorker.cs:2627 does precede the runner construction at 2657, so the "reconnect or restart: no extra drops" claim holds on ordering.
  • DarlingXeSessions.cs now has exactly one GetAzureDatabaseListAsync(server, databaseScope: null call, which is what CollectorDatabaseScopeRungTests was changed to assert.
  • The two missing-space-after-the-assignment slips from the earlier commits are gone at this head — the net diff against the base touches neither LongQueriesDisabledWarning.Visibility line.
  • Every test the body names by name exists and asserts what the body says it asserts (spot-checked all of the "guards which pass at both commits" list plus the three round-3 Darling source pins).

One small bookkeeping slip in the body: TheSweep_PassesTheRegistrationsAndTheServersOwnList_FromTheLiveRegistry is listed both as a new guard and under "existing tests that changed". It is new in this PR.

Could not be checked from a read-only tree: the full-suite counts (Lite 7181/0, Darling 19904/0/1320), the 0-build-warnings claim, the per-commit RED/GREEN sequencing, and the Microsoft Learn facts about read-only replicas sharing one session definition.

claude[bot]
claude Bot previously requested changes Oct 2, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: CHANGES REQUESTED

Two findings, both inline. The claim-by-claim notes that do not fit here are in the top-level comment.

Claims

  • VERIFIED — one shared rule, both apps compute the same sets, and no create or drop touches master or a separately monitored database — LongQueryTraceDatabases.cs:72-95 is the only Plan; CanTouch at lines 78-80 applied at 87 and 92; called from DarlingXeSessions.cs:736 and RemoteCollectorService.LongQueryCompletions.cs:212.
  • VERIFIED — while the trace is off the exclusions and the scope do not apply, and the lifecycle shares the read loop's scope resolver — new applyExclusions: false overloads at DarlingCollectorRunner.cs:6591 and RemoteCollectorService.cs:1060; DarlingCollectorRunner.cs:31 consumed at DarlingXeSessions.cs:577.
  • VERIFIED — a failed listing now throws, Lite logs a failed drop at Warning, and only a cleanup pass that ran resets the count — DarlingXeSessions.cs:584-586; RemoteCollectorService.LongQueryCompletions.cs:547-563; retry.Reset() inside the drop branch at line 127, not beside MarkLongQueryTraceApplied at 130; the pass guard at DarlingWorker.cs:4188.
  • VERIFIED — the registry is published before the runner is built (DarlingWorker.cs:2627 vs 2657), KeptElsewhere ignores read-only intent while AzureMasterScope.cs:49 honours it, the two whitespace slips are gone at this head, and every test the body names exists and asserts what the body says.
  • UNVERIFIED — the full-suite counts and the 0-build-warnings claim — needs a Windows build and test run; these are net10.0-windows WPF projects and this tree is read-only Linux.
  • UNVERIFIED — the per-commit RED/GREEN ledger — only the merged head is present; reproducing it needs those commits built.
  • UNVERIFIED — "a drop on the primary removes it from the replicas too (inferred)" and the Learn facts about the session definition reaching replicas — external docs plus a live Azure tier. The body labels the first "inferred", which is honest.
  • REFUTED — "while the trace is on, it also runs the create side once an hour", read with "after 5 failed cleanup passes in a row … the app stops retrying on every cycle" as the whole retry story. A CreateOnly pass that throws is re-selected on every sweep, uncapped: the general catch at DarlingWorker.cs:4231-4252 leaves LongQueryTraceApplied, LongQueryTraceAppliedKey and LongQueryTraceAppliedAtUtc untouched, so the gate at 4168-4172 stays true forever. The pass then repeats at sweep cadence with a warning and one connection per monitored database each time. The cap arms only off LongQueryTraceDropException; the create side has none.

Riskiest lines

  1. Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs:4168 — the hourly-create gate reads a stamp no failure path advances, so the one new cadence degrades to per-sweep on exactly the failure it exists to recover from — UNPINNED (On_ASessionDroppedFromOutside_IsCreatedAgainAnHourLater_AndNothingIsDropped and On_WithinTheHour_TheLatchedReconcileRunsNothing drive only the succeeding pass)
  2. PerformanceMonitor.Collectors/LongQueryTraceDatabases.cs:92 — the single predicate behind every session drop; an error in any of its three terms strands a session or destroys a co-owned one — pinned by Off_DropsTheSessionEverywhere_ExcludedAndOutOfScopeDatabasesIncluded, On_ADatabaseMonitoredAsItsOwnServer_AndScopedOut_IsNotDropped, On_TheOutsideDrop_StillDropsADatabase_ThatBothLogicalServerRegistrationsExclude and the Lite twins
  3. Lite/Services/RemoteCollectorService.LongQueryCompletions.cs:82 — gates separatelyMonitored, registrations and KeptElsewhere, while lines 197 and 395 re-read the same live mutable status and re-decide the Azure branch for themselves — UNPINNED (every rig sets the edition once in its constructor and never moves it)

Traps near this diff

  • "A default interface member compiles a parity gap away … CONTRIBUTING's two-store parity rule names IAlertDeliverer; its members are required." — does not apply — the new defaulted member is on ICollectorDefinition<TRow> (ICollectorDefinition.cs:54), not a seam CONTRIBUTING names; that interface already carries the same shape at line 118, CollectorDefinitionBase.cs:84 supplies the virtual, and the inherited answer is the correct one for every other collector — the one needing the opposite overrides at LongQueryCompletionsCollector.cs:145.
  • "Lite and Darling ship in parity … a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply — both SKUs got the plan, the read skip (DarlingCollectorRunner.cs:62-65 / RemoteCollectorService.DefinitionRunner.cs:285-288), the banner (ViewerServerTab.LongQueries.cs:21-27 / ServerTab.LongQueries.cs:21-25), the retry cap, the schedule text and a mirrored test class; the remaining asymmetries are ones the stores genuinely differ on and each is commented.
  • "An expression-bodied member holding a { } property pattern is read SHORT by the T-SQL convention guard's member walk (#3667) — a member the walk cannot read whole is one no census can see into." — applies and is handled — both new ListLongQueryTraceDatabasesAsync members are that shape and both are in KnownTruncatedRanges with the reason inline (TsqlConventionGuardTests.cs:1460-1465, 1529-1531); re-deriving DeclarationEnd (CSharpMemberMap.cs:333-404) shows it skips braces inside parentheses, so the new DropLongQueryTraceInEachAsync in each app is read whole and correctly absent.
  • "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — does not apply — the varying sentence is aliased through LongQueryCompletionsCollector.SessionScopeSentence (LongQueryCompletionsCollector.cs:154-156), and each app's test asserts the literal rather than the other app's copy (Lite.Tests/LongQueryBannerSessionScopeTests.cs:48, Darling.Tests/LongQueryBannerSessionScopeTests.cs:42).
  • "OPTION(RECOMPILE) on collector queries … A statement added to an existing batch needs its own hint — one on a neighbouring statement does not cover it." — does not apply — no collector query is added. The only new T-SQL path is the unchanged guarded drop via BuildDropSessionSql (RemoteCollectorService.LongQueryCompletions.cs:594), and the sys.databases enumeration at RemoteCollectorService.cs:1122 is pre-existing text whose only change is which parameters the exclusion filter binds — still fully parameterised, and BuildDatabaseExclusionFilter at line 1241 already handled the new null.

Divergence (blind read vs PR body)

1. The retry story has a hole its own machinery does not cover. Every failure path the body enumerates is a cleanup failure, and the cap, the hourly clock and the Debug downgrade are all written against LongQueryTraceDropException. The body then adds a second recurring pass, the hourly create, and says only what it does when it succeeds. My blind read of DarlingWorker.cs:4154-4252 says a create pass that throws leaves every input to its own gate unchanged, so it reruns on the next sweep and the one after, forever. "Darling's hourly create pass: No. It only creates." is true and is not the risk; the cadence claim is, and neither the body nor any test addresses it. This drives the verdict.

2. The body treats "is this Azure SQL Database?" as one fact; Lite's code reads it three times. The body is careful that the two lists agree, and separatelyMonitored really is threaded into the callees. But the predicate those lists hang off is re-derived at three sites from the live mutable connection status, and the first bypasses KnownEngineEditions, the one class written because the raw live edition is unreliable right after a start or a failed check (KnownEngineEditions.cs:34-37). "The same set that the alert sweep uses" is true of the list and not of the gate in front of it.

Smaller non-blocking notes are in the top-level comment. Everything else matched, including the claims I expected to find overstated.

Reviewer checklist

  • Reproduce the uncapped create retry on a live Azure SQL Database target: latch the trace on, revoke ALTER ANY EVENT SESSION (or break the master hop), and watch the log for an hour. Expect one reconcile warning per sweep rather than one per hour.
  • Confirm the full-suite runs and the 0-build-warnings claim on Windows with the .NET 10 SDK — neither test project builds on this read-only Linux tree.
  • Settle the read-only-replica question on a real tier with read scale-out on: does a drop on the primary remove the definition from the replicas? The body calls this "inferred" and KeptElsewhere rests on it.
  • Open a Lite server tab on an Azure SQL Database registration whose connection status is not yet populated and confirm the first reconcile does not take the Azure drop path with empty registrations.
  • Look at the Long Queries tab of a master registration on a logical server with separately registered databases whose own traces are off, and decide whether the missing databases need a note the way the always-on lists got one.

… read once, the verb's search and the exclusion clause

The new tests that pin behavior still to come fail at this commit. The seams that hold the exclusion
clause of each app's Azure database list, and the verb's search plan, are in place with the behavior
the code had before.
…ition once, --drop-xe-sessions searches excluded databases

Darling: an hourly create pass, or the attempt after the cap, that throws something other than a failed
drop moves its hourly clock, so later sweeps do not repeat it and its warning.
Lite: the reconcile resolves the engine edition once and passes it to the ensure and the drop.
Darling --drop-xe-sessions: for the long-query session it searches every online database, the exclusions
not applied, except master, a database monitored as its own server and a database another registration
of the server keeps it in. It counts every other registration as keeping it, because it cannot read their
schedules. The other sessions keep the monitored list.
Removes LongQueryTraceDropRetry.ConsecutiveFailures, which nothing read.
Comment thread Darling/PerformanceMonitor.Darling.Service/DarlingXeSessions.cs
@claude

claude Bot commented Oct 2, 2026

Copy link
Copy Markdown

Divergence: the --drop-xe-sessions change is in the diff and not in the body

I read the diff before the body. Everything the body describes about the lifecycle checks out — but one changed product surface, and two changed test files, appear nowhere in it.

The body's scope sentence is: "This PR changes where Lite and Darling create and drop that session, and it corrects the text that describes it." The only mention of the verb is under Text: "Darling's --drop-xe-sessions prints a warning, and the README quotes it. Both now say that Darling creates a missing long query completions session again within an hour."

What the diff also does to that verb, unmentioned:

  • SqlServerXeSessionCleanupTarget grows a registry parameter, and ConnectXeSessionCleanupTargetAsync / the injected connect delegate change arity to carry it (DarlingCliCommands.cs:6054, 6141, 6151). The verb now hands the target the servers it resolved the target in.
  • A new PlanAzureSearch splits the search into two sets: the always-on sessions keep the monitored list, while the long-query session is searched over every online database with the exclusions not applied, minus master, minus databases monitored as their own servers, minus databases another registration keeps (DarlingXeSessionCleanup.cs:502-547). FindSessionsAsync filters each name through plan.Reports (line 587) so a session found in a database outside its own set is not reported.
  • The verb's usage text and the README's verb paragraph are rewritten accordingly (DarlingCliCommands.cs:5906-5907, README.md:2201) — not the collector paragraph the body quotes.
  • Three new tests in DropXeSessionsVerbTests (TheLongQuerySearch_VisitsAnExcludedDatabase_AndSkipsOneMonitoredAsItsOwnServer, TheLongQuerySearch_SkipsADatabaseAnotherRegistrationOfTheServerKeeps, ATargetWithItsOwnNames_SearchesNoDatabaseForTheLongQuerySession), plus the ThrowingConnect / (server, _, _) arity updates.
  • TargetProviderTests gains TheRunnersAzureDatabaseList_AppliesTheServersExclusionsOnlyWhenAsked.

Neither DropXeSessionsVerbTests nor TargetProviderTests is in any of the three rounds' "Existing tests that changed" lists.

This matters beyond bookkeeping: the one finding I'm requesting changes for lives in exactly that unreviewed surface (inline on DarlingXeSessionCleanup.cs:577). A reader working from the body would not know the verb now opens databases the registration excludes, and would not think to ask what a failure there does to the exit code.

Two smaller things, neither blocking:

  • LongQueryCompletionsCollector.cs:37 still reads "enabling creates the session on the monitored servers, disabling DROPS it there" with no Azure caveat. Every other description of the same fact got one in this PR — CollectorScheduleDefaults.cs:76, ScheduleManager.cs:754, collection_schedule.json:20, DarlingCliCommands.cs:5117, the README, both banners. This is the class doc of the collector that owns SessionScopeSentence, so it is the one place a reader would check first.
  • Darling.Tests/LongQueryBannerSessionScopeTests.cs:240-243 carries its own RepoPath / RepoRoot([CallerFilePath]) walk and reads with File.ReadAllText rather than going through RepoFile. RepoFileAdoptionTests explicitly scopes itself out of this ("It says nothing about the fifty-three classes here that carry their own repo-ROOT walk"), so nothing reds — it is the duplication that census exists to discourage, one layer down. The sibling file added in the same PR (LongQueryTraceLifecycleTests.cs) does use the shared reader.

claude[bot]
claude Bot previously requested changes Oct 2, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: CHANGES REQUESTED

Claims

  • VERIFIED — one shared rule, LongQueryTraceDatabases, so both apps compute the same sets — Plan called from DarlingXeSessions.cs:727/:740 and RemoteCollectorService.LongQueryCompletions.cs:218/:408/:532
  • VERIFIED — the off-side drop ignores exclusions and scope — applyExclusions: false reaches the query in both apps (DarlingCollectorRunner.cs:6666, RemoteCollectorService.cs:1149); Plan's if (!enabled) arm never reads monitoredDatabases
  • VERIFIED — Darling's lifecycle takes the #3477 scope from the read loop's own delegate — DatabaseScopeFor returns _databaseScope(...) (DarlingCollectorRunner.cs:1184), passed at DarlingXeSessions.cs:947; pinned by CollectorDatabaseScopeRungTests:499-500
  • VERIFIED — SkipsSeparatelyMonitoredDatabases is true only on this collector, and both read loops filter on it — LongQueryCompletionsCollector.cs:170; defaults at ICollectorDefinition.cs:51, CollectorDefinitionBase.cs:84; filters at DarlingCollectorRunner.cs:2235, RemoteCollectorService.DefinitionRunner.cs:285
  • VERIFIED — 5 failed passes, one Warning naming the databases, then hourly at Debug — DropAttemptCap, LongQueryTraceDropRetry.RecordFailure/RetryDue, GiveUpWarning, the afterTheCap log switch in both DropLongQueryTraceInEachAsync
  • VERIFIED — Lite's count resets only after a cleanup pass that ran and succeeded — retry.Reset() inside the drop branch (RemoteCollectorService.LongQueryCompletions.cs:135, :145), not beside MarkLongQueryTraceApplied; pinned by On_AfterTheCap_TheCyclesInBetween_KeepTheHourlyAttempt
  • VERIFIED — Darling's hourly pass only creates, and does not reset the cleanup count — CreateOnly returns before the outside-drop; if (pass != LongQueryTracePass.CreateOnly) Reset() at DarlingWorker.cs:4188
  • VERIFIED — the state key tells scope from exclusions — four level separators in LongQueryTraceDatabases consumed by StateKey/CoOwners; pinned by TheStateKey_TellsWhereTheScopeEndsAndTheExclusionsBegin
  • VERIFIED — the registry is published before the runner is built, so the first reconcile sees the co-owners — _registryState.Publish(initialServers) at DarlingWorker.cs:2627, runner at :2644, both reads (:4058, :2668) live on top
  • VERIFIED — the quoted banner/README/CLI/description text is what the tree carries, and the = spacing matches dev — LongQueryCompletionsCollector.cs:151-153, Darling/README.md:741, DarlingCliCommands.cs:5117, ScheduleManager.cs:754, collection_schedule.json:20, ServerTab.LongQueries.cs:40, ViewerServerTab.LongQueries.cs:39
  • UNVERIFIED — "a drop on the primary removes it from the replicas too (inferred)" and the read-only-intent reasoning on it — needs a read-scale-out database: drop on the primary, read sys.database_event_sessions on the replica
  • UNVERIFIED — the suite counts (7181/0, 19904/0/1320) and the per-commit RED-to-GREEN narrative — both suites are net10.0-windows WPF; this ran on Linux against a read-only tree, so nothing was built or run

Riskiest lines

  1. Darling/PerformanceMonitor.Darling.Service/DarlingXeSessionCleanup.cs:577 — plan.Visited now includes the excluded databases, and a failure to open one becomes a Problems entry that forces exit 2 (:381) even when every session found was dropped — UNPINNED (TheLongQuerySearch_VisitsAnExcludedDatabase_... pins the plan, not the exit code)
  2. Darling/PerformanceMonitor.Darling.Service/DarlingXeSessions.cs:724 — the enable-side listing failure went return null to throw: right for the retry, but uncapped at two warnings per sweep where the drop-side twin three lines down is capped — retry pinned by On_AFailedListing_IsNotMarkedApplied; cadence UNPINNED
  3. PerformanceMonitor.Collectors/LongQueryTraceDatabases.cs — Plan's listedDatabases.Where(d => CanTouch(d) && !keep.Contains(d) && !kept.Contains(d)): a wrong kept leaves sessions forever or removes one another registration reads — pinned by Off_LeavesTheDatabaseOfAReadOnlyRegistration_WhileItsTraceIsOn, On_TheOutsideDrop_StillDropsADatabase_ThatBothLogicalServerRegistrationsExclude and their Lite twins

Traps near this diff

  • "A default interface member compiles a parity gap away ... left Lite's deliverer and fourteen fakes inheriting an answer nobody wrote (#3640)" — applies in shape, not as a defect: the default body is at ICollectorDefinition.cs:51, but both runners read the member (DarlingCollectorRunner.cs:2235, RemoteCollectorService.DefinitionRunner.cs:285) and the only collector needing another answer overrides it (LongQueryCompletionsCollector.cs:170)
  • "An expression-bodied member holding a { } property pattern is read SHORT by the T-SQL convention guard's member walk (#3667)" — applies, handled: the two new ones are rostered with the reason (TsqlConventionGuardTests.cs:1465, :1531). Nothing else new has the shape — DropLongQueryTraceInEachAsync's braces sit at paren depth 1, which DeclarationEnd skips (CSharpMemberMap.cs:357-359), and the local KeptElsewhere(...) => has no access modifier for DeclarationHead to match (:84-89)
  • "Lite and Darling ship in parity ... a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit (#3594)" — applies and holds: shared planner, read skip, applyExclusions overload (RemoteCollectorService.cs:1060 / DarlingCollectorRunner.cs:6591), cap, both banners, both descriptions. Asymmetries are app-justified — Lite has no scope (...LongQueryCompletions.cs:108) and needs no hourly create because it ensures every cycle
  • "InternalsVisibleTo on PerformanceMonitor.Analysis names Lite.Tests only — a helper Darling.Tests must call has to be public (#3688)" — does not apply: the new internals are in the Viewer and Lite, each visible to its own suite (PerformanceMonitor.Darling.Viewer.csproj:66, PerformanceMonitorLite.csproj:44), and the cross-project helper arrives by Compile Include (Lite.Tests.csproj:42)
  • "A test that reads the clock twice measures the CI scheduler (#3676). One anchor owns both ends of an interval" — does not apply: neither rig reads a real clock — utcNow is a parameter (DarlingWorker.cs:4127) and Lite injects LongQueryTraceUtcNowForTests (...LongQueryCompletions.cs:116)
  • "Integer division in a rate column ... truncates every rate under 1/s to 0", and the rest of the SQL family — do not apply: the only T-SQL touched is moved verbatim into its own member (RemoteCollectorService.cs:1153), exclusions still parameterized through BuildDatabaseExclusionFilter; no new query, no arithmetic, no ordinal sort

Divergence (blind read vs PR body)

The body's lifecycle account matches the diff on every claim graded above, including the two dev defects it names. The gap is scope: it says the PR "changes where Lite and Darling create and drop that session, and it corrects the text that describes it", and mentions --drop-xe-sessions only for one warning sentence. The diff also rewrites where that verb searches — SqlServerXeSessionCleanupTarget takes the registry, PlanAzureSearch splits the search into an always-on set and a long-query set built with the exclusions not applied, FindSessionsAsync filters each found name through plan.Reports, and the connector plus the injected connect delegate change arity. Three new DropXeSessionsVerbTests and one new TargetProviderTests go with it, and neither file appears in any round's "Existing tests that changed" list. That is where the finding lives rather than a bookkeeping complaint: a reader working from the body would not know the verb now opens databases the registration excludes, so would not ask what a failure there does to the exit code. Details and two non-blocking notes are in the top-level comment.

Reviewer checklist

  • Build both suites on Windows and run them — the counts, the 0-warning claim and the RED-to-GREEN narrative are what a read-only Linux tree cannot touch
  • On a real Azure SQL Database logical server, exclude one database the login cannot open (or a paused serverless one), turn the trace on, run --drop-xe-sessions <server>, and record the exit code and stderr
  • With the trace off there, watch one Darling reconnect and count the connections the off-side drop opens: it now visits every online database, exclusions included
  • Drop the session on the primary of a read-scale-out database and read sys.database_event_sessions on the replica, to settle the inferred claim
  • With the trace on and a login that cannot read master, watch Darling's log and judge the two-warnings-per-sweep cadence

… no longer fails the verb; document the create-side retry cadence

The long-query search opens databases the registration excludes, so one the login cannot open
(no user there, a paused serverless database) made the verb exit 2 even after it dropped every
session it found. Such a database is now a note on stderr that names it and says a session left
there needs a manual drop, and the exit code is unchanged. A monitored database that cannot be
opened still exits 2. The usage text and the README say so.

The Azure create side's comment now says why its listing failure rethrows and retries on every
sweep, and that only the drop side is capped.
Comment thread PerformanceMonitor.Collectors/ICollectorDefinition.cs
Comment thread Lite/Services/RemoteCollectorService.LongQueryCompletions.cs
Comment thread Lite/Services/RemoteCollectorService.BlockedProcessReport.cs
Comment thread PerformanceMonitor.Collectors/LongQueryCompletionsCollector.cs

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: LGTM

Claims

  • VERIFIED — the rule lives in one place and both apps compute the same sets — LongQueryTraceDatabases.cs:83 is the only Plan; callers at RemoteCollectorService.LongQueryCompletions.cs:218/:428/:552, DarlingXeSessions.cs:260/:303/:385, DarlingXeSessionCleanup.cs:580.
  • VERIFIED — off-side drop ignores exclusions and scope — applyExclusions: false / databaseScope: null at RemoteCollectorService.cs:1151, DarlingCollectorRunner.cs:6668, :6682; pinned by Off_DropsTheSessionEverywhere_ExcludedDatabasesIncluded and ..._ExcludedAndOutOfScopeDatabasesIncluded.
  • VERIFIED — a separately-monitored database is neither created, dropped nor read by the logical server — CanTouch at LongQueryTraceDatabases.cs:77; read cut by LongQueryCompletionsCollector.cs:144 at RemoteCollectorService.DefinitionRunner.cs:285 and DarlingCollectorRunner.cs:2237.
  • VERIFIED — no drop removes a session another registration keeps, whatever its read-only intent — OtherOwners filters on Enabled && TraceOn && Host only; pinned both directions by Off_LeavesTheDatabaseOfAReadOnlyRegistration_WhileItsTraceIsOn and ReadOnlyRegistration_Off_LeavesItsDatabase_WhileTheLogicalServersTraceIsOnAndCoversIt.
  • VERIFIED — "the coverage is exact" for two registrations that both exclude a database — Keeps checks the owner's own exclusions; pinned by On_TheOutsideDrop_StillDropsADatabase_ThatBothLogicalServerRegistrationsExclude.
  • VERIFIED — each state-key part has its own separator — four control characters at LongQueryTraceDatabases.cs:~40; pinned by TheStateKey_TellsWhereTheScopeEndsAndTheExclusionsBegin.
  • VERIFIED — Darling's hourly create pass, drop only on connect / key change / after-cap — DarlingWorker.cs:4158 ladder, DarlingXeSessions.cs:377 early return; pass reaches only the Azure arm (:597).
  • VERIFIED — a failed listing now throws instead of returning — DarlingXeSessions.cs:289; pinned by On_AFailedListing_IsNotMarkedApplied.
  • VERIFIED — Lite logs a failed drop at Warning now, not Debug — RemoteCollectorService.LongQueryCompletions.cs:~565.
  • VERIFIED — Lite's count resets only after a cleanup pass that ran and succeeded — retry.Reset() only at :135 and :145; pinned by On_AfterTheCap_TheCyclesInBetween_KeepTheHourlyAttempt.
  • VERIFIED — Lite reads the edition once and remembers it — :90 plus KnownEngineEditions.cs:134; pinned by Off_AnEditionThatChangesDuringTheReconcile_StillLeavesTheDatabasesAnotherRegistrationKeeps, which drives the blank-then-5 flip through the clock hook.
  • VERIFIED — the verb searches excluded databases for that session only, with a second master listing only when exclusions exist — DarlingXeSessionCleanup.cs:605, PlanAzureSearch at :573.
  • VERIFIED — LongQueryTraceDropRetry.ConsecutiveFailures is gone — no such member in the tree.
  • VERIFIED — SkipsSeparatelyMonitoredDatabases is true only on this collector — sole => true at LongQueryCompletionsCollector.cs:144.
  • VERIFIED — both Visibility = lines match dev at this head — no diff line for either; the =await form exists only in intermediate commits.
  • VERIFIED — both truncated members added to KnownTruncatedRanges — TsqlConventionGuardTests.cs:1460, :1529, with the stranded text named.
  • UNVERIFIED — every RED/GREEN and pass-count figure — test-run outputs; no SDK runs here and Lite/Viewer are WPF. I confirmed each cited test exists and reads on the behaviour claimed.
  • UNVERIFIED — "0 build warnings" — needs a build. The one readable risk: this PR makes cref="GetAzureDatabaseListAsync" at BlockedProcessReport.cs:315 ambiguous, but no project sets GenerateDocumentationFile, so CS0419 cannot fire.
  • UNVERIFIED — "full suites not run at this head" — self-declared disclosure, not checkable from the tree.
  • UNVERIFIED — the Learn read-scale-out facts and the body's own inferred "a drop on the primary removes it from the replicas too".

Riskiest lines

  1. PerformanceMonitor.Collectors/LongQueryTraceDatabases.cs:83 — drops over a list with exclusions and scope deliberately off; CanTouch and kept are all that prevent removing a session another registration reads — pinned by Off_LeavesTheSessionOfADatabaseMonitoredAsItsOwnServer and the two Off_Leaves... co-owner tests in both SKUs.
  2. Lite/Services/RemoteCollectorService.LongQueryCompletions.cs:90 — one edition read drives both halves, via a KnownEngineEditions never seeded from the store; a blank status takes the server-scoped arm, as engineEdition == 5 did on dev — pinned by Off_AnEditionThatChangesDuringTheReconcile_StillLeavesTheDatabasesAnotherRegistrationKeeps.
  3. Darling/PerformanceMonitor.Darling.Service/DarlingWorker.cs:4158 — the pass ladder; wrong here is a drop that never runs or a warning every sweep — pinned by On_WithinTheHour_TheLatchedReconcileRunsNothing, On_ASessionDroppedFromOutside_IsCreatedAgainAnHourLater_AndNothingIsDropped, and the two ...RunsAgainAnHourLater_NotOnEverySweep tests.

Traps near this diff

  • "Lite and Darling ship in parity … a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply — every arm lands twice (banner ServerTab.LongQueries.cs:21 / ViewerServerTab.LongQueries.cs:17; read cut :285 / :2237; plan; cap; applyExclusions seam RemoteCollectorService.cs:1149 / DarlingCollectorRunner.cs:6665). The two single-SKU items are stated: Darling's hourly create pass (Lite ensures every cycle, RemoteCollectorService.cs:542) and --drop-xe-sessions (Lite has no such verb).
  • "A default interface member compiles a parity gap away: DeliverAndReportAsync with a default body left Lite's deliverer and fourteen fakes inheriting an answer nobody wrote (#3640)." — does not apply — ICollectorDefinition.cs:54 has a default body, but the gap is a host that never asks, and both hosts ask (:285, :2237). Non-blocking comment left: the body is dead anyway, CollectorDefinitionBase.cs:84 binds.
  • "An expression-bodied member holding a { } property pattern is read SHORT by the T-SQL convention guard's member walk (#3667)" — applies, and is handled — both new ListLongQueryTraceDatabasesAsync members are entered at TsqlConventionGuardTests.cs:1460 / :1529 with reasons.
  • "OPTION(RECOMPILE) on collector queries … A statement added to an existing batch needs its own hint" — does not apply — no new T-SQL; the enumeration moves verbatim into RemoteCollectorService.cs:1153 and SqlServerTargetProvider.cs:148 is unchanged.
  • "Truncation inferred from a full page … Fetch limit + 1" — does not apply — no paged read or MCP payload; LongQueryTraceDropException.Databases carries the names that failed, not a count.
  • "The engine must remember the observation stamp it last fired on, per key, beside — never folded into — its last-seen stamp" (#3579/#3636) — does not apply as a defect; this is that shape done right, LongQueryTraceDropRetry keyed on StateKey and separate from the applied latch.
  • "A test that reads the clock twice measures the CI scheduler (#3676). One anchor owns both ends of an interval." — does not apply — both rigs inject the clock (LongQueryTraceUtcNowForTests, Rig.Clock).
  • "CASE WHEN interval > 0 THEN delta / interval ELSE 0 END spells 'unknowable' as 0" — does not apply — no rate, delta or aggregate anywhere in this diff.

Divergence (blind read vs PR body)

None that changes a verdict. Three narrower readings, none a defect:

  • The body calls LongQueryTraceAppliedAtUtc "the time of the last pass that created the session"; it is stamped on every successful pass including the off-side one (DarlingWorker.cs:4188). Harmless — CreateOnly is gated on enabled (:4167).
  • "Cross-registration: No" reads as one guard; it is two with different coverage — CanTouch/ownedElsewhere for a read-write separate registration, KeptElsewhere for a read-only one (which AzureMasterScope omits). I walked all four on/off combinations of both shapes and found no hole.
  • The verb's self-exclusion by selfId holds only because DarlingServerConnector.cs:426 stamps ServerId from config.ServerId and DarlingCliCommands.cs:6156 passes the same resolved list. On the store-unreachable fallback the ids collapse and nothing is kept — failing toward dropping more, the safe direction. Unmentioned, not wrong.

Reviewer checklist

  • Build both projects and run the two full suites at this head — the body discloses they were not run, and this LGTM rests on test names, not a green run.
  • Live Azure logical server with a database also registered as its own server: toggle off then on, confirm sys.database_event_sessions matches the monitored set and the separate database is untouched both ways.
  • The read-only-intent case on a tier that has a real read-only replica — "a drop on the primary removes it from the replicas too" is marked inferred and is what makes KeptElsewhere's intent-blindness correct.
  • A logical server where every user database is registered separately: watch one long_query_completions run row, since the list is then empty and a zero-row read is the shape this codebase fights.
  • --drop-xe-sessions with an unopenable excluded database (note on stderr, exit 0) and an unopenable monitored one (exit 2).
  • Kill a database-scoped session under a running Darling service; confirm it returns within the hour, not at the next reconnect.

@erikdarlingdata
erikdarlingdata dismissed stale reviews from claude[bot] and claude[bot] October 2, 2026 19:13

Its findings are fixed at 5712458, where the review reads LGTM.

@erikdarlingdata
erikdarlingdata marked this pull request as ready for review October 2, 2026 19:21
@erikdarlingdata
erikdarlingdata merged commit caaf0ed into dev Oct 2, 2026
20 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/long-query-banner-azure-db branch October 2, 2026 19:21
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