Part of #3099 - #3108
Part of #3099#3108
Conversation
A collector's binary COPY into the store that fails on a transport fault gets one re-attempt on a fresh store connection. The batch is still the method's own parameter and a failed COPY commits nothing, so the re-attempt is lossless. A server reply is never re-attempted: PostgresException means the backend received the statement and answered, so an identical second attempt gets an identical answer. The predicate is shared with the managed store's post-start connection loop, which asks the same question of the exception under a different retry policy. The status stays SUCCESS. The cycle's re-attempt count reaches the collection_log row through the existing note channel, so a write that faulted and then succeeded is distinguishable from one that never faulted.
The pin took the source between WriteBatchAsync and CopyBatchOnceAsync and asserted the re-attempt call appeared in it. A variant where the write called something else stayed green, because the slice swallowed anything declared in the gap and matched that instead. It now counts every call to the COPY and requires each one to sit inside the re-attempt's own argument list, and reads the fresh-connection claim off the rewrite delegate alone.
The both-attempts-fail test asserted an empty sink. Both of its delegates throw before touching anything, so the sink was empty however RunAsync behaved -- coverage that reads as a claim and checks nothing. The claim that survives is that no value is returned at all, which is the throw.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…roken A transport fault breaks the store connection the caller holds, and on a fan-out that connection is shared across every batch. So each batch after the first faulting one inherited a connection it could not use, and what that throws need not be a transport fault the re-attempt recognises. plan_correction is why it matters rather than being tidiness: it enumerates on every non-Azure target and declares no WatermarkColumn, so it both fans out and cannot re-read a sample it loses that way. The decision reads the connection's own state instead of a later exception's shape. Both fresh-connection paths route through one named method so "fresh" has one body.
|
Reviewed the diff (Darling-only: Main finding (posted inline on Other things checked, no issues found:
|
The re-attempt is at-least-once. A transport fault raised before the commit acknowledgment is awaited leaves the store untouched, but a fault raised while awaiting it can leave the server committed while the client sees a retryable exception, and the second attempt lands the batch again. IncludesCollectionId defaults true, so each attempt stamps its own row ids and nothing downstream can collapse the copies. collection_id is in no continuous aggregate's GROUP BY, so both copies land in the same bucket and 18 sum() views double it, materialised, past the four-day raw window, and inherited by each daily rollup off its hourly view. sample_count doubles with it on five of the eighteen, which is the signature to look for.
"Eight readers" counted three prose mentions of the predicate as readers. The query clauses are seven, across five files: Lite's collection-health read, the self-alert evaluator's last_success and recent_success, both MCP readers, and the viewer's two.
|
Reviewed the diff ( Correctness — traced carefully, no bugs found:
Lite/Darling parity: No parity gap — this is Npgsql/PostgreSQL-transport-specific (Lite writes to an embedded, in-process DuckDB with no network transport to fault on), so there's no Lite counterpart that should have received an equivalent change. The new Minor scope note (not a bug): One nit posted inline: an unused No security or T-SQL style issues — this PR touches no T-SQL and no user-facing input handling. |
Re-running the batch re-runs WritePayload, which is not a pure function of its row: it derives deltas through CollectorDeltaCalculator, which advances its baseline as a side effect of being asked. A second pass over rows the first attempt already rendered therefore commits a zero for every delta column -- on eight collectors, three of them the null-watermark ones the re-attempt is for. A zero delta reads as an idle interval, carries no error and never self-corrects, which is worse than the lost sample. #3095's StoreCopyPhase separates the two cases. A start-phase fault came out of BeginBinaryImportAsync, so no row was started -- a COPY FROM STDIN cannot have committed and no WritePayload ran. IsSafeToReattempt requires that value positively, so a data-phase fault and an unstamped fault both decline and cost a sample rather than authorising a duplicate or a fabricated zero. That also makes the re-attempt exactly-once, so the at-least-once window and the aggregate-doubling it implied no longer exist.
|
Reviewed the full diff (all 7 commits) against the merged Correctness — traced the final state of
Parity — this only touches Darling's Postgres COPY write path (transport-fault handling that has no Lite analogue, since Lite writes to a local embedded DuckDB with no equivalent network transport-fault surface). The Security — no new external input handling, SQL construction, or secrets. Pure connection/retry orchestration. Style — no T-SQL in this PR. C# additions have copyright headers and follow existing file conventions (named log/note templates, No correctness, parity, or security issues found. The test suite ( |
IsSafeToReattempt's second conjunct means something only because Start is in force until BeginBinaryImportAsync returns and can never apply to a fault raised once the row loop has begun. That is a property of the COPY block in another file, and its pin lives in a third, so an auditor reading the gate had no way to find either. Both directions now name each other: the gate points at the stamping site and the test that pins it, and StoreCopyPhase.Start and Stamp say that a re-attempt decision is downstream, so anyone widening the try or adding a stamping call site knows what it authorises.
…e pair The stamping pin constrained transitions to Data and explicit Stamp(ex, Start) calls, but never bare assignments to Start. A `copyPhase = StoreCopyPhase.Start;` added below the row loop passed all five assertions -- and since IsSafeToReattempt re-runs a batch on Start, that made a post-row-loop fault re-attemptable: a duplicated write and a delta column of zeros, with every test green. A symmetric count closes it. The instance pair covers the class a source pin cannot. A pin catches edits of shapes it anticipated; the hole above was a shape nobody did. These run the real path and read the stamp off a real exception, so a construction that produces the wrong phase fails even when every regex still matches. The start case needs no store -- an unopened connection makes the real BeginBinaryImportAsync refuse before it reaches a socket. The data case needs Begin to succeed and is gated on DARLING_TEST_PG.
|
Reviewed this against CONTRIBUTING.md's C# conventions, Lite/Darling parity, and the usual correctness/security/perf lenses. No .sql files are touched, so the T-SQL style section doesn't apply here. Scope/parity: Darling-only, as expected — this is a binary-COPY transport-fault retry for the PostgreSQL store write path, which has no Lite analogue (Lite's DuckDB writes are local/embedded, not subject to the same network transport faults). Correctness: Traced the two-axis gate (
Minor/non-blocking: Test coverage ( No correctness, parity, security, or performance issues found. |
The two instance tests reach the shared DARLING_TEST_PG store, which made StoreCopyPhaseTests a live class and tripped LivePostgresCollectionHygieneTests. That pin's preferred remedy is this shape: a mostly-pure class keeps its purity and the live tests move to their own ...LivePostgresTests. Putting the attribute on StoreCopyPhaseTests instead would serialize seven source-text pins against every other live class for nothing. The vacuity guards travel with the tests: the start case asserts the row loop was not reached, the data case asserts Begin did return. The start case needs no store and stays with the data case anyway -- they are a test and its negative control, and they share the probe plumbing. Both phase counts now spell the assignment \s*=\s* rather than with literal single spaces. copyPhase=StoreCopyPhase.Start; is valid C# and evaded every literal form.
LiveCleanupConversionRatchetTests flagged the probe helper's finally. Its remedy is LiveStoreCleanup, which is for teardown that deletes rows on the body's own connection -- and this block deleted nothing. The COPY always faults, so no attempt commits a row and there is nothing to clean. Removed the construct instead of wrapping it. The helper takes its connection and data source, both callers hold them under `await using`, and disposal is the language's job. That closes the hazard the ratchet exists for rather than routing around it: a DisposeAsync in a finally can throw and REPLACE the body's exception, which on this helper is the entire result being measured.
|
Reviewed the diff (Darling-only: Scope / parity — No Lite counterpart is needed here. The retry targets a network binary Correctness — Traced the gate end to end:
Nothing here looks unsound, and the test suite ( Minor nit (non-blocking) — No security, injection, or performance concerns — this is exception-classification and connection-management logic, no user input reaches it, and the added overhead (one |
1 participant
Implements the first of #3099's three suggested directions: a collector's binary COPY into the store that fails on a transport fault in the COPY's start phase gets one re-attempt on a fresh store connection. Before this there is no retry anywhere on the path — the
retry/backoffhits inDarlingCollectorRunnerare #2776's plan-fetch width backoff and #2673's budget-abandon, both meaning "the next scheduled cycle".Part of #3099, notFixes. This touches the collector COPY write path only. #3099 is evidenced across a wider population — its own report shows the same error text on store reads (Collection-health self-alert failed,Failed to check forced-plan failures), which this does not touch and which lose an alert evaluation rather than a sample. The closing condition belongs on the issue, not inherited from a title here.The gate is the design, not a precaution
WriteBatchAsyncsplits into a wrapper andCopyBatchOnceAsync, which holds the COPY and (for the #1767 diverting collectors) the transaction wrapping it and its dimension flush. The split is load-bearing: the importer and the transaction must be disposed before a second attempt, or it re-enters with an aborted transaction and fails on 25P02 rather than on anything to do with the store.StoreWriteReattempt.IsSafeToReattemptANDs two independent axes, and both are required:SocketException/IOException/TimeoutExceptionanywhere in the inner chain, elseNpgsqlExceptionat the top.PostgresExceptionanywhere in the chain is the backend answering (duplicate key, missing relation, bad input, full disk, its ownstatement_timeoutat 57014); an identical second attempt gets an identical answer, and re-attempting past a SQLSTATE turns a legible error into a silent one. The chain is walked with thePostgresExceptiontest first at every level, so wrapping cannot smuggle a SQLSTATE past the predicate.StoreCopyPhase.Start— required positively.Why the phase axis is what makes this correct. Re-running the batch re-runs
WritePayload, andWritePayloadis not a pure function of its row: it derives deltas throughCollectorDeltaCalculator, whoseAddOrUpdatereturns(currentValue, …)on every arm — so asking for a delta advances the baseline as a side effect. A second pass over rows the first attempt already rendered therefore computescurrentValue - currentValue = 0and commits a zero for every delta column. That affects 8 collectors (QueryStats8 calls,FileIoStats8,ProcedureStats7,Spinlock4,LatchStats3,WaitStats3,MemoryGrants2,PerfmonStats1) — three of them the null-watermark collectors this re-attempt exists for. A zero delta reads as a genuinely idle interval, carries no error, and never self-corrects; that is strictly worse than the lost sample.StoreCopyPhase.Startis the one state where neither hazard exists. It is raised byBeginBinaryImportAsyncitself, so the importer never returned: no row was started, hence aCOPY … FROM STDINcannot have committed, andWritePayloadnever ran, hence no baseline moved. #3095 puts theStart→Datatransition inside the COPY block precisely soStartcannot mean anything looser.So the re-attempt is exactly-once, not at-least-once. There is no lost-commit-ack window and no aggregate duplication to disclose, because the only phase it fires in cannot have committed.
Unknowndeclines, and that is deliberate. It is what an unstamped exception reads as, and it covers the dimension flush and the transaction commit after the COPY (#1767) — which genuinely can commit. RequiringStartpositively means a missing stamp costs a sample instead of authorising a duplicate. A predicate written as "notData" would re-attempt everything it failed to recognise; that variant is pinned red.What it recovers, at its real size
Honest sizing, because only the cost was quantified before:
WatermarkColumncollector's watermark isMAX(column)over rows already stored, so a failed write cannot advance it and the next cycle re-reads the same range.Smaller on both sides than the first draft of this description implied, and it should be argued at that size. What does not shrink is the character of the loss: on a null-watermark collector the sample is unrecoverable, and the data is sitting in memory when the write fails.
The base default in
CollectorDefinitionBaseisWatermarkColumn => null; the affected set isquery_stats,procedure_stats,file_io_stats,plan_cache_stats,plan_correction.Cancellation
Both attempts run on the caller's
cancellationToken, and a token already cancelled suppresses the re-attempt entirely.That token is the service's stopping token, not a deadline, so honouring it costs nothing in the population this exists for. What it prevents is a re-attempt outliving an orderly stop while holding a sweep permit and a store connection, against a bundled store the same process is shutting down — the one case where a second attempt is guaranteed useless. The suppression sits in the exception filter rather than as a throw inside the arm, so a write failing during a stop still reports its own transport fault instead of being relabelled a cancellation. Accepted consequence: a write that fails because of shutdown loses its sample exactly as today.
OperationCanceledExceptionis caught and rethrown ahead of the filter rather than left to the predicate answering false — an arm whose correctness rests on a predicate not matching is one predicate edit away from re-attempting through a shutdown.No delay, and why none is purchasable
The mechanism is the fresh connection, not a wait: the first attempt's connector is dead — the reasoning
DarlingManagedPostgres's post-start loop already records — so a second COPY on the caller's handle would fail on the protocol rather than on the store.A delay is not available here at any useful length. The sweep permit and the caller's borrowed store connection are both held for the duration, so a pause long enough to outlast the store's own hourly continuous-aggregate refresh (hundreds of seconds, per #3099's duration series) would hold both for minutes, while any pause short enough to be safe is noise against that window. The accepted cost is that a write's worst case is two command deadlines rather than one.
How it is recorded
The status stays SUCCESS. A new value would be read as a failure by every consumer of
status IN ('SUCCESS', 'SKIPPED')— seven query clauses across five files (Lite's collection-health read, the self-alert evaluator'slast_successandrecent_success, both MCP readers, and the Darling viewer's two) — and would suppresslast_successfor a cycle that stored every row and advanced its watermark. That is #2673's defect with the sign flipped, and the same reasoning that put the whole-cycle-budget message in the note channel rather than inventing a sixth status.The cycle is distinguished by two things:
collection_logrow, composed into the existing Enumerated collectors that yield zero items log SUCCESS indistinguishable from healthy #1837 note channel throughEnumeratedCollectorDriver.MergeNotes. Merged rather than assigned, read once after every write on all three dispatch paths, so a re-attempt cannot displace the probe-failure or partial-database note it can co-occur with. A count rather than a flag because the fan-out paths write once per database or per item.Recording it is not a courtesy: a transport fault currently writes a
collection_logERROR row, and those rows are the measurement behind #3099's in-window versus out-window rate ratio and the prediction registered against it. A silent retry would remove the lost sample and the instrument that checks whether the association it was diagnosed from is real.Known cost
The caller's connection stays broken for the rest of the cycle, and on a fan-out it is shared across every batch (#2819 has the Query Store plan and text fetches borrowing the same one). Each batch after the faulting one detects it by state and sends its first attempt to a fresh connection — so there is no wasted first attempt and no second Warning line; a faulted cycle logs one Warning, for the batch that faulted. Deciding on the connection's own state rather than on a later exception's shape or stamping keeps those batches out of the phase question entirely.
The cost is connection holds. For the remainder of a faulted cycle that server holds two store connections at a time rather than one: the caller's broken handle, still checked out until its
await usingunwinds, plus the fresh one each batch borrows. #2819's re-derivation ofMaxPoolSize = 24sized the pool at the sweep width on peak concurrent holds being one per swept server, so this is the 2x multiplier that bound moved away from — reachable only if many swept servers fault in the same cycle. Repairing the caller's handle in place would need the callers to hold a re-openable connection: wider than this change.plan_correctionis why the state check matters rather than being tidiness: it enumerates on every non-Azure target and declares noWatermarkColumn, so it both fans out and cannot re-read a lost sample.WriteBackfillBatchAsyncinherits the re-attempt, since it routes throughWriteBatchAsync. Its context is not attached to aCollectorRunResult, so its note reaches nothing; the Warning line still fires.Scope
Direction 1 only. #3095's start-phase deadline is merged (#3110) and this consumes its phase axis rather than duplicating it. Refresh scheduling is untouched.
Tests
StoreWriteReattemptTests, 20 cases. The policy runs throughStoreWriteReattempt.RunAsyncwith attempt delegates that move rows into a sink, because the COPY it wraps is reachable only through a live store and a constructed runner. What delegates cannot see — that the shipped write routes through it at all, and that the re-attempt takes a fresh connection rather than the caller's dead one — is pinned against the source. Neither half is sufficient alone.Phase stamps in the tests are applied through the shipped producer (
CollectorFaultCopyPhase.Stamp), not by writingException.Datadirectly, so the key asserted is the key the runner writes rather than a retyped string.Every assertion is red-proofed against a mutation that keeps the tree compiling:
ADataPhaseFaultIsNeverReattempted,AnUnstampedFaultIsNeverReattempted,IsSafeToReattemptRequiresBothAxes!= Datainstead of== StartAnUnstampedFaultIsNeverReattempted,IsSafeToReattemptRequiresBothAxes(theDatatest stays green — the two declining phases are pinned separately)AServerReplyIsNeverReattemptedrows +AWrappedServerReplyIsNeverReattemptedAFailedFirstWriteFollowedByASuccessfulReattempt_StoresTheRows_AndReportsSuccessAFailureOnBothAttempts_ReportsTheError— "nothing was thrown"TheShippedStoreWriteRoutesThroughTheReattemptOnAFreshConnectionpgConnectionCopyOnAFreshConnectionAsyncstops opening its own connectionTheReattemptCountReachesTheCollectionLogNoteThe gate's other half lives in #3110, and both directions now say so
IsSafeToReattempt'sStartconjunct means something only becauseStartis in force untilBeginBinaryImportAsyncreturns and can never apply to a fault raised once the row loop has begun. That is a property of the COPY block, pinned in a third file, and an auditor reading the gate could find neither. So the gate's doc now names the stamping site andTheCopyWriteStampsTheStartPhaseUntilBeginReturns, andStoreCopyPhase.StartandStampnow say a re-attempt decision is downstream — because whoever widens thattryor adds a stamping call site is the person who needs to know.A hole in that pin, closed
The pin constrained transitions to
Data(exactly one, correctly positioned) and explicitStamp(ex, Start)calls (exactly zero). It never constrained bare assignments toStart. I confirmed the counts in the merged runner —copyPhase = StoreCopyPhase.Start;occurs 1,Stamp(ex, StoreCopyPhase.Start)occurs 0 — then proved it exploitable: addingcopyPhase = StoreCopyPhase.Start;below the row loop left all 75 tests green, while making a post-row-loop fault carryStart. Since the gate re-runs the batch onStart, that is a duplicated write and a delta column of zeros with nothing red.A symmetric count assertion closes it, red-proofed with that same mutation (1→2). Both counts and both positional patterns now spell the assignment
\s*=\s*rather than with literal single spaces —copyPhase=StoreCopyPhase.Start;is valid C# and evaded every literal form; that variant is red-proofed too.And the instance pair, because the pin and the instance test fail differently
Not instead of the pin — neither covers the other's blind spot, and I have that in both directions rather than as an argument:
= Start;below the row loopBeginhoisted outside the stampedtryStart→UnknownA source pin catches every edit of a shape it anticipated and is blind to constructions nobody thought of, which is precisely what the hole above was. The instance test runs the real path and reads the stamp off a real exception, so a construction producing the wrong phase fails even when every regex matches.
The two instance tests live in
StoreCopyPhaseLivePostgresTestsunder[Collection("live-postgres")], not inStoreCopyPhaseTests. They reach the sharedDARLING_TEST_PGstore, which makes their class a live class — andLivePostgresCollectionHygieneTestsprefers exactly this remedy: a mostly-pure class keeps its purity and the live tests move out, rather than serializing seven source-text pins against every other live class. The vacuity guards travel with the tests.The start case needs no store — an unopened connection makes the real
BeginBinaryImportAsyncrefuse before it reaches a socket, which is a start-phase fault — so it runs everywhere and I red-proofed it locally. Its definition'sWritePayloadthrows a distinctive exception, so a reachable row loop would be named rather than silently changing what the test measures. The data case needsBeginto succeed, so it is gated onDARLING_TEST_PGand its first execution is CI's live job; I could not run it on macOS and am not claiming otherwise. It is the negative control that stops the start case passing under an unconditionalStartstamp.The server-reply rows are stamped
Starton purpose: the phase axis would accept them, so their decline can only be the type axis doing its job. Unstamped they would pass for the wrong reason.The rows-storing assertion checks the sink rather than the return value alone — a helper returning the right count while writing nothing would satisfy a count-only assertion, and "no sample is lost" is a claim about the store's contents.
CollectionSweepCommandTimeoutTests(the sweep's own deadline scanner) andDarlingPayloadProbeFailureTestsrun against the change and stay green.PayloadDimensionTests' exactly-one-occurrence-and-ordering pin onimporter.CompleteAsync/PayloadDimensionWriter.FlushAsync/transaction.CommitAsync, andDarlingEmptyEnumerationNoteTests' whitespace-collapsed argument-list pin, were verified by hand against the shipped source — both classes need the WPF Viewer project and cannot compile in a macOS harness.CHANGELOG entry