Repository navigation
Hand Lite's analysis token to the store reads it is meant to abandon (#2443) - #2454
Merged
Merged
Conversation
IPlanFetcher.FetchPlanXmlAsync is the only call on the analysis pass that opens a session on a MONITORED SQL Server rather than on the monitoring store, which makes it the one an abandoned pass most needs to be able to let go of -- and it was the one call with no token at all. Its own sibling on the same class, FetchPlanBySqlHandleAsync, has taken one since it was written, so a cancelled fetch kept a session alive or not depending on which key the caller happened to have. Threading it means moving the interface, and the interface moves with all three implementers and all three call sites in one commit or none of them compile. The parameter is REQUIRED rather than defaulted, which is the substance of this commit rather than a detail of it. A defaulted token would have let every existing call site keep passing nothing while the signature said the fetch was abandonable -- the exact shape #2443 exists to remove -- and requiring it is what surfaced the real find here: DarlingWorker's fetch_plan command already had a cancellationToken in scope and was already passing it to the by-sql_handle arm, and simply did not pass it to the by-plan_handle arm two lines above. Nothing was defended against that; the compiler found it the moment the default was refused. PgPlanFetcher's catch stops swallowing the abandonment it is now handed, for the same reason the by-sql_handle arm has excluded cancellation since it was written: an abandoned fetch is not a plan that failed to come back, and logging it as one would both add an ERROR for work we called off and let the drill-down carry on enriching under a token that had already fired. The deprecated Dashboard implementation moves too. It is still in the solution and still built by CI, so leaving it behind would have been a red build rather than a decision. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
146 store calls across the fact collector and the drill-down collector took the no-argument overload, which is why #2438's budget could abandon a pass between stages but never inside a read. A wedged query held its connection until it finished on its own, and the service that had already given up on that server sat there holding a session it was no longer waiting for. Threading the reads alone would have accomplished almost nothing, and that is the part worth reading twice. The fact collector's per-query catches are bare `catch { }` on purpose -- a missing table degrades to "no facts", never an exception -- so a fired token would have been caught thirty times over and the pass would have carried on to the next collector each time, dialling the store again to be told again. The measured proof is in the behavioural test: reverted, it fails with an NpgsqlException instead of an OperationCanceledException, which is the collector connecting to the store while holding a token that had already been cancelled before the call. So all 27 of those catches now carry the classification the rest of the pass has used since #2299 -- `when (!AnalysisShutdown.IsExpectedAbandon(ex, ...))` -- and an abandonment propagates to the single line the pass logs for it instead of being swallowed per-metric. The catch bodies and their comments are unchanged; the only thing that stops being caught is the one thing we asked for. Four collect methods have no catch at all and never did: their reads propagate straight up, which is the same outcome by a shorter route. AppendDmvSnapshotRowsAsync takes a required token rather than a defaulted one for the reason the plan fetcher did: three callers share it, two are on the pass, and a default would have let either keep passing nothing silently. The viewer is the third and it genuinely has no pass to abandon -- it says so at its own call site rather than in the shared method, because the exception belongs to the caller. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…written" The finding store is the one place on this pass where cancellation abandons a WRITE rather than a read, and #2443 asked for that to be decided rather than swept along with everything else. It is decided here in the direction that produces fewer outcomes, not more. The mute-filter read gets the token like every other read: an unreadable mute registry fails open by design, which is right, but "the mutes could not be read" and "we stopped reading" are different answers and only the first should fail open. Swallowing the second would have let the pass go on to enrich and persist an unfiltered finding set under a token that had already fired. The insert does not, and its connection open is deliberately the last abandonment point on the pass. A half-written finding set is a third outcome, and unlike the timeout and the shutdown that #2430's classifier names, this one is invisible. There is no transaction -- per-row failure isolation is the Dashboard twin's posture, one bad row logs and the batch continues -- every row in a batch shares one analysis_time, and GetLatestFindingsSql keys on MAX(analysis_time). So a batch cut in half does not read as truncated. It reads as a complete analysis that found fewer problems, which means the server looks HEALTHIER for having been abandoned, with nothing anywhere marking the set incomplete. Cancelling before the first row costs this cycle's findings and says so in the line the pass already logs; cancelling after it quietly changes what the store means. Cancelling inside a single row would be worse still and buys nothing measurable: Npgsql's cancel is a request to the server, so a cancelled INSERT can leave a row that did land, in a set nothing marks as partial -- an ambiguous commit in exchange for milliseconds of loopback work. The tail is affordable to finish: N small INSERTs against the local managed store, which is not where a pass wedges. This is the same call DarlingAnalysisService made a layer up in #2299 ("the post-enrichment tail carries no check"), restated at the write it protects so the next person to read it finds the reasoning at the code rather than two files away. The read-back surfaces -- the viewer's findings and mute reads, the MCP mute verb, the retention sweep -- are exempt for a different reason and are marked as such one by one: they are off the pass entirely, serving lifetimes with no per-pass budget and no wedged analysis, so there is no token to thread and inventing one would be pretending. Note that a partial set is still reachable today from a store FAULT mid-batch, which this commit does not change and which is filed separately; what it does change is that the cancellation path can no longer reach it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The threading is mechanical; this is the part that lasts. A sweep of 168 call sites only stays swept if something counts it, and the specific way this one would regrow is known in advance: add `CancellationToken cancellationToken = default` to a signature, stop there, and every existing call site keeps passing None while the code LOOKS threaded. So the pin is on the CALL and not the signature. A no-argument ExecuteReaderAsync() is what goes red, and no default can hide it. It scans the whole analysis project directory rather than a file list, because a file list is a thing a seventh PgFactCollector partial can be added outside of. The exemptions are enumerated by method name with a reason each, and the reason also has to appear in the method's own doc comment -- the enumeration is here so a new one cannot arrive silently, and the marker is there so the reason travels with the code. The exemption list this repo spent a day removing was a wildcard, and the way it grew was that nobody ever had to name a new entry. A second test asserts the reverse direction too, because an exemption that no longer corresponds to an untokened call is a claim the code stopped making, and it must not sit there authorising the next one. Three of the five are behavioural rather than textual. The fact-collector catch pin is what stops the token being silently useless -- a bare catch there swallows the abandonment and the pass keeps collecting under a fired token. The collection test runs a pre-cancelled pass with no store at all: reverted, it fails with an NpgsqlException instead of an OperationCanceledException, which is the defect stated exactly -- handed a token that had already fired, the collector still dialled. The insert test pins the abandonment point itself, including the absence of a between-rows check, because adding one there would MAKE the partial set rather than prevent it and that is not obvious from reading the loop. All five go red with the change reverted, verified by reverting it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…l as the room AcquireWriteLock has taken a TimeSpan? since it was written; AcquireReadLock took nothing and blocked in EnterReadLock() until it got in. That asymmetry is the one place in #2412's story where cancellation genuinely could not be replaced by better reporting, and it is why the observability half of #2419 was load-bearing rather than a fallback. The reason it matters is the ORDER of a store read on the analysis pass. Every one of them takes this lock first and opens its connection second, so a pass queued behind a long archival or a compaction was uninterruptible no matter how carefully its reads were threaded -- the token would have reached the reads and stopped at the door. Threading the reads without this would have produced a pass that could be abandoned everywhere except where it was actually stuck. A token rather than a timeout, and that is the substance of the choice. A timeout needs a NUMBER, and there is no honest one to pick here: the right budget for waiting on this lock is exactly the caller's remaining budget, which the caller already holds and which nothing in this class can know. Handing it the token says that directly. ReaderWriterLockSlim has no token-taking overload, so a poll is the only way to say it. The interval is only ever reached under contention -- an uncontended TryEnterReadLock returns immediately -- and CancellationToken.None takes the original EnterReadLock() path unchanged, so the fourteen callers outside the analysis pass keep exactly the behaviour they had rather than being quietly re-timed by a change they did not ask for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…name #2419 put the judgement inline, in one catch, as `catch (OperationCanceledException ex) when (context.CancellationToken.IsCancellationRequested)`. That was right for one site. Threading the store layer needs the same judgement at forty-odd catch sites, and forty copies of a two-clause predicate is how the clauses drift, so it moves to AnalysisAbandon.IsExpected unchanged -- signalled token AND a cancellation shape, both halves still load-bearing. The TYPE half especially. Since #2419 this token fires on an ordinary timeout, so it is signalled during perfectly normal running; a filter asking only "has the token fired?" would relabel any genuine fault landing after the budget elapsed as an abandonment and swallow the one line of evidence it left -- at forty sites at once instead of one. That was #2419's first review finding and it gets worse, not better, with scale. The shape list is one entry long because that is what was MEASURED. Darling's classifier also names a disposed data source and the 57P0x trio because its store is a separate postmaster that can vanish under an in-flight read; Lite's is an embedded file in this process, and the concern was that DuckDB's interrupt would surface as a DuckDBException needing its own pattern-match. It does not. Against DuckDB.NET 1.5.5 on net10.0: a pre-cancelled token throws OperationCanceledException without touching the database; a token fired mid-query reaches duckdb_interrupt through DuckDBCommand.Cancel() and also surfaces as OperationCanceledException; a 2,790 ms query cancelled at 2,000 ms aborted at 2,004 ms; and both the interrupted connection and a fresh one against the same file were fully usable afterwards. So naming the one shape loses nothing the cancellation actually produces, and widening it would cost fault evidence. The data-span probe is the first read the pass makes and the first to be threaded. Its bare `catch { return 0; }` had to go with it: a probe failure reading as "no data yet" is right, but an abandonment reading that way turns a cancelled pass into an insufficient-data SKIP, which is a different and far calmer-looking answer than the one the caller is about to log. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…es eating it 188 store calls across the fact collector, the drill-down collector, the anomaly detector and the baseline provider took the no-argument overload, which is why #2419's budget could abandon a pass at a stage boundary but never inside a read. A collector that had already started ran to completion, holding the read lock and the connection of a pass that had stopped waiting for it. Threading the reads alone would have accomplished almost nothing, and that is the part worth reading twice. The fact collector's 27 per-query catches are bare `catch { }` on purpose -- a missing table degrades to "no facts", never an exception -- so a fired token would have been caught 27 times and the pass would have carried on to the next collector each time, taking the lock again to be told again. #2419's per-collector ThrowIfCancellationRequested bounds that to one wasted attempt per collector, which is much better than nothing and still 27 of them. So every one of those catches now classifies through AnalysisAbandon.IsExpected, and an abandonment propagates to the single line AnalysisService logs for it instead of being swallowed per collector. The catch bodies and their comments are unchanged; the only thing that stops being caught is the one thing we asked for. Four collect methods have no catch at all and never did -- their reads propagate straight up, which is the same outcome by a shorter route. The nine AnomalyDetector catches are the loudest of the lot, because they do not swallow, they log AppLogger.Error. Unfiltered, an ordinary budget expiry would have produced nine ERROR lines describing nine detectors that "failed". The BaselineProvider catch is the same shape and is the one that produced five of the seven lines #2299 was filed about on the Darling side. AppendDmvSnapshotRowsAsync takes a required token rather than a defaulted one: three callers share it, two are on the pass, and a default would have let either keep passing nothing silently. The viewer is the third and genuinely has no pass to abandon -- it says so at its own call site rather than in the shared method, because the exception belongs to the caller that has it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hing was written" The same decision as the Darling twin's, reached the same way, because the two finding stores have the same shape and a divergence here would be a parity bug rather than a local choice. The mute-filter phase gets the token like every other read. The insert does not, and its lock acquisition and connection open are deliberately the last abandonment points on the pass. A half-written finding set is an outcome that reads like a healthy one: there is no transaction, every row in a batch shares one analysis_time, and GetLatestFindingsAsync takes the newest analysis_time -- so a batch cut in half does not read as truncated, it reads as a complete analysis that found fewer problems. The server looks HEALTHIER for having been abandoned, with nothing marking the set incomplete. Cancelling before the first row costs this cycle's findings and says so in the line the pass already logs; cancelling after it quietly changes what the store means. Cancelling inside a row would be worse and buys nothing: DuckDB's cancel is duckdb_interrupt, so an interrupted INSERT can leave a row that did commit, in a set nothing marks as partial -- an ambiguous commit in exchange for microseconds of in-process work. And this restates at the write the same call AnalysisService made a layer up in #2419 ("the post-enrichment tail carries no check on purpose"), so the reasoning sits at the code rather than two files away. The four read-back surfaces -- the recommendations reader, the viewer's latest-run findings, the MCP mute verb, the retention sweep -- are exempt for a different reason and are marked one by one: they are off the pass entirely, serving lifetimes with no per-pass budget and no wedged analysis, so there is no token to thread and inventing one would be pretending. A partial set is still reachable from a store FAULT mid-batch, which this does not change and which is filed as #2448 against both SKUs; what it does change is that the cancellation path can no longer reach it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The twin of Darling.Tests' pin, guarding the same regrowth: add `CancellationToken cancellationToken = default` to a signature, stop there, and every existing call site keeps passing None while the code LOOKS threaded. So the assertion is on the CALL. A no-argument ExecuteReaderAsync() is what goes red, and no default can hide it. It scans the whole Lite/Analysis directory rather than a file list, because a file list is a thing a seventh DuckDbFactCollector partial can be added outside of. AcquireReadLock() is on the forbidden list beside the ADO calls rather than in a check of its own, and that placement is the point. Every store read on this pass takes the lock BEFORE it opens its connection, so a pass whose reads had tokens and whose lock did not would still be uninterruptible behind an archival -- the two are one property and splitting them into two tests would let half of it be reverted quietly. The exemptions are enumerated by method name with a reason each, and the reason also has to appear in the method's own doc comment: the enumeration is here so a new one cannot arrive silently, the marker is there so the reason travels with the code. A second test asserts the reverse direction, because an exemption that no longer matches an untokened call is a claim the code stopped making and must not sit there authorising the next one. Two of the six are behavioural. The classifier truth table pins both halves of the predicate, including the one that matters most -- a fault that merely coincides with the budget expiring is still a fault. The read-lock test holds a real write lock on another thread, which is what an archival looks like from here, and asserts the read side gives up when asked; with the pre-change EnterReadLock() spliced back in it does not fail, it HANGS until the writer lets go, which is the defect stated as precisely as a test can state it. Verified by reverting: the four source pins go red, and the read-lock test cannot even be compiled against the old signature. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both findings check out and the first one is the substantive one. I wrote in the PR description that the IPlanFetcher move was "signature and call-site edits only -- no Lite behaviour changes here". That is not true for cancellation semantics, and review said so precisely. Lite arms a real per-pass budget (#2412/#2419), the token now reaches SqlPlanFetcher's OpenAsync and ExecuteScalarAsync through DrillDownCollector.Plans.cs, and its catch was unconditional -- so the first time a Lite pass abandoned mid-fetch it would have logged "Failed to fetch plan for handle ...: The operation was canceled." as an ERROR for work we ourselves called off, and swallowed it, letting the pass carry on enriching under a fired token instead of unwinding to AnalysisService's one quiet line. That is the exact pattern this PR spends its diff removing on the Darling side, reintroduced in Lite by the change that made the token real. The arm matches PgPlanFetcher's for the identical call shape. The deprecated Dashboard implementation has the same unconditional catch and is left alone: its scheduler calls AnalyzeAsync without a token at all, so nothing there can fire and adding a filter would imply a cancellation that cannot happen. The second finding is a doc-comment defect in my own insertion. Six #2443-exempt <para> blocks in PgFindingStore were appended to the END of the preceding summary line rather than starting on their own, so the literal "///" became part of the doc text and would have rendered as stray slashes. Worth more than its size: this is the file whose exemptions are the argument for the whole sweep, and a reader who does not trust the formatting will not trust the reasoning either. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The same doc-comment defect review found in the Darling twin, in the same four places for the same reason: each #2443-exempt <para> block was appended to the END of the preceding summary line instead of starting on its own, so the literal "///" became part of the doc text and would have rendered as stray slashes. Worth more than its size in this file specifically. These four exemptions are the argument for withholding the token from a whole surface, and a reader who does not trust the formatting will not trust the reasoning either. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 21, 2026
erikdarlingdata
added a commit
that referenced
this pull request
Aug 21, 2026
#2465) Twenty CA2016 warnings, five call sites, one file. The analyzer only started saying this once #2454 gave AcquireReadLock a token overload, and what it is pointing at is real rather than stylistic: every method in this service takes the DATABASE LOCK BEFORE it opens its connection, so a token that reaches the reads and not the lock is a token that stops at the door. That is exactly the hole #2454 found on the analysis pass -- abandonable everywhere except where the pass is actually stuck -- and a startup repair queued behind a long archival has the same shape. So the five are decided one at a time, and the split is by what abandoning COSTS rather than by whether the site reads or writes. THREE FORWARD IT. The marker read asks a question and writes nothing. The survey is the longest read here -- a full GROUP BY over query_store_stats and then a read_parquet of every monthly archive file -- and is precisely the thing that should yield to an archival rather than queue behind it. The archive rewrite-to-temp is the expensive one, and abandoning it lands in a state the design already handles rather than a new one: the original is untouched by construction, the marker is withheld, and the next launch retries a repair the pre-fix signature makes idempotent. Each of the three abandons into a state the next launch reproduces for free. TWO DECLINE IT, in CancellationToken.None with the reason at the site, per the convention #2454 set for its fourteen off-pass callers. Both are the marker write, and the marker is the one thing here that records work already done. The post-repair one is the sharper case: the hot table is collapsed and committed and every affected archive file has been swapped, and abandoning the record does not undo any of it -- it only makes the next launch pay the whole survey to rediscover there is nothing left to repair. And reaching either line proves the token was unfired a moment earlier, since the survey (or the entire repair) just returned under it, so forwarding would buy an abandonment window microseconds wide at that price. The declines go all the way through -- the lock, the OpenAsync and the two statements -- rather than stopping at the lock. #2454's InsertFindingsAsync declines only past the lock and the open, and it is right to, because what it must not cut is a multi-row batch that would read as a complete analysis. The marker write has no such shape; what it must not lose is the record itself. A lock that will not be abandoned in front of a write that will be is worse than either choice made whole. Not blanket-suppressed, and the write locks are not swept in with the rest. CA2016 cannot see AcquireWriteLock because there is no token-taking overload, but the better reason to leave those two alone is that they are the genuinely maintenance-shaped acquisitions -- the hot-table mutation with its CHECKPOINT, and the instant a live path's bytes are replaced -- which is the one category #2463 does not put in question. The pin is a source guard because what is being pinned is a DECISION and not a behavior: MainWindow fires this un-awaited with no token, so all five spellings run identically today and nothing observable separates a forward from a decline. That is the reason they need pinning rather than a reason they do not -- an edit that tidied the two declines into forwards, or suppressed the rule for the file, would cost nothing at runtime and quietly turn five stated decisions back into an unknown. The declines are pinned WITH their reason, since a bare CancellationToken.None is the oversight the analyzer complained about with one more token typed in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the other half of #2443. Darling is #2449; this branch sits on top of it so it compiles, and the diff narrows to Lite's own commits the moment #2449 merges. Merge #2449 first.
What was wrong
#2419 armed a per-pass budget and put a
ThrowIfCancellationRequestedahead of every store-touching stage, which closed the user-visible defect in #2412 — a wedged server became loudly stuck instead of silently skipped forever. Its diff touched no store file. So abandonment happened between stages: a collector that had already started ran to completion, holding the read lock and the connection of a pass that had stopped waiting for it.Re-derived count on
dev: 203 untokened async store calls across 17 files inLite/Analysis/—ExecuteReaderAsync65,ReadAsync65,OpenAsync67,ExecuteScalarAsync3,ExecuteNonQueryAsync3. (The issue estimated ~138. Two of the 203 areSqlPlanFetcher's SqlClient calls, which #2449 already threads; the DuckDB side is 201.) Plus 66AcquireReadLock()calls in the same files, which turn out to matter more than any of them.The lock was the real hole
AcquireWriteLockhas taken aTimeSpan?since it was written;AcquireReadLocktook nothing and blocked inEnterReadLock()until it got in. And the order of every store read on this pass is: take the lock, then open the connection. So a pass queued behind a long archival or a compaction was uninterruptible no matter how carefully its reads were threaded — the token would have reached the reads and stopped at the door. This is the item #2443 called out as "Lite-only, small, and the one place where the reporting genuinely cannot be replaced by cancellation", and threading the reads without it would have produced a pass abandonable everywhere except where it was actually stuck.It takes a token, not a timeout, and that is the choice rather than a detail: a timeout needs a NUMBER, and the right budget for waiting on this lock is exactly the caller's remaining budget — which the caller already holds and nothing in
DuckDbInitializercan know.ReaderWriterLockSlimhas no token-taking overload, so a 50 ms poll is the only way to say it; the interval is only reached under contention, andCancellationToken.Nonetakes the originalEnterReadLock()path unchanged, so the fourteen callers outside the analysis pass are not quietly re-timed.DuckDB cancellation, measured rather than assumed
#2419 recorded that a token cancelled at 2 s aborted a 15,233 ms query at 2,004 ms. What it did not record is what that throws, and the whole classifier depends on it — the worry was a
DuckDBExceptionneeding its own pattern-match. Measured against DuckDB.NET 1.5.5 on net10.0:ExecuteScalarAsync(ct)TaskCanceledException, database never touchedExecuteReaderAsync(ct)System.OperationCanceledException— 2,790 ms query aborted at 2,004 msReadAsync(ct), token fired mid-loopSo
duckdb_interruptreaches .NET as a plainOperationCanceledExceptionand there is nothing else to name.AnalysisAbandon.IsExpectedis one shape long because that is what was measured, not because it was assumed — and widening it would cost fault evidence for nothing.Threading the reads alone would have accomplished almost nothing
The fact collector's 27 per-query catches are bare
catch { }on purpose — a missing table degrades to "no facts", never an exception — so a fired token would have been caught 27 times and the pass would have carried on to the next collector each time. (#2419's per-collector checkpoint bounds that to one wasted attempt per collector: much better than nothing, and still 27 of them.)The nine
AnomalyDetectorcatches are worse, because they do not swallow — they logAppLogger.Error. Unfiltered, an ordinary budget expiry would have produced nine ERROR lines describing nine detectors that "failed".BaselineProvider's is the same shape, and is the one that produced five of the seven lines #2299 was filed about on the Darling side.So all 40-odd now classify through
AnalysisAbandon.IsExpected, and an abandonment propagates to the single lineAnalysisServicelogs for it. Catch bodies and comments are unchanged; the only thing that stops being caught is the one thing we asked for.AnalysisAbandonexists because #2419's inline predicate was right for one site and forty copies of a two-clause predicate is how the clauses drift. Both halves stay load-bearing, and the TYPE half especially: this token fires on an ordinary timeout, so it is signalled during normal running, and a filter asking only "has the token fired?" would relabel any fault landing after the budget as an abandonment — #2419's own first review finding, at forty sites instead of one.What cancellation means, per caller
DuckDbFactCollector.*DrillDownCollector.*AnomalyDetector/BaselineProviderFindingStore.FilterMutedFindingsAsyncFindingStore.InsertFinding*A partial persist cannot happen on the cancellation path
Identical to the Darling twin's decision, reached the same way — a divergence here would be a parity bug, not a local choice. The lock and the connection open are the last abandonment points; past them the batch finishes.
There is no transaction, every row in a batch shares one
analysis_time, andGetLatestFindingsAsynctakes the newestanalysis_time— so a batch cut in half does not read as truncated. It reads as a complete analysis that found fewer problems, and the server looks healthier for having been abandoned. Cancelling inside a row would be worse:duckdb_interruptcan leave a row that did commit, in a set nothing marks as partial — an ambiguous commit for microseconds of in-process work. A partial set is still reachable from a store fault mid-batch; that predates this work and is filed as #2448 against both SKUs.The 15 exemptions
Four read-back surfaces (the recommendations reader, the viewer's latest-run findings, the MCP mute verb, the retention sweep) plus the finding INSERT — the same five the Darling twin exempts, for the same two reasons. Each is exempt by method name in the test and carries a
#2443 exemptnote in its own doc comment, with a second test asserting the reverse direction.The test
Lite.Tests/AnalysisPassTokenThreadingTests, six tests.AcquireReadLock()sits on the forbidden list beside the ADO calls rather than in a check of its own, deliberately: the lock and the reads are one property, and splitting them into two tests would let half of it be reverted quietly.Two are behavioural. The read-lock test holds a real write lock on another thread — what an archival looks like from here — and asserts the read side gives up when asked. Reverted, it does not fail, it hangs until the writer lets go, which is the defect stated as precisely as a test can state it.
Verification
EnableWindowsTargeting), 0 errors, no new warnings. Each of the five Lite commits builds clean on its own, checked individually.Lite.Testsisnet10.0-windowsand cannot run on macOS, so the new test file was compiled into anet10.0xUnit harness together with the realLite/Analysis/AnalysisAbandon.csandLite/Database/DuckDbInitializer.cssources (one harness-only stub for the single service-layer symbolDuckDbInitializerreaches for — a pure hash on a legacy re-key path that nothing under test calls). 6/6 pass. WithLite/Analysisreverted, the four source pins go red; with the pre-changeEnterReadLock()spliced back in, the read-lock test hangs rather than passing. CI is the arbiter for the rest of the suite.Deferred
FindingStore.InsertFindingsAsynctakes a read lock for a write, and has since it was written. Out of scope for a token sweep and not touched here; filed separately.Co-Authored-By: Claude Opus 5 noreply@anthropic.com