Diagnostic pipeline reliability: value-channel errors, fail-loud deferred registry, trace persistence (#311) - #326
Merged
Conversation
…d deferred IDs (#311) - InternalError and NavigationTargetNotFound added to s_deferredDescriptors; QRY063 was a 4th shipped-unregistered deferred ID (raw string at ChainAnalyzer.ResolveNavigationColumn) beyond the three the issue lists. - Both deferred report loops now route through ReportDeferredDiagnostic: unregistered IDs report a QRY900 naming the ID instead of silent continue. - DeferredDiagnosticRegistryTests pin the registrations and the null-resolve fallback contract.
…rorBag (#311) - Stage 3 outputs BindStageResult (bound site OR BindFailure); failures flow to a dedicated Collect()+report node emitting QRY900 at the failing site. Survives incremental caching, thread switches, and files whose only site failed to bind (which previously produced no interceptor group and thus no report at all). - Bind catch now excludes OperationCanceledException instead of recording cancellation as an error. - ChainAnalyzer's three catch handlers report deferred InternalError diagnostics (registered in the previous commit) instead of the bag. - PipelineErrorBag deleted; both drain sites removed. QRY900 'path 2' is no longer dead code. - CallSiteBinder.TestThrowOnMethodName test hook + BindFailureDiagnosticTests exercise the path end-to-end (acceptance criterion 1).
…RACE at emission (#311) - TraceCapture is now produced and consumed within a single orchestrator call: trace lines are captured onto AssembledPlan.TraceLines (equality- excluded, like MySqlBindOrder) right after SQL assembly, and the ThreadStatic is cleared in a finally so cancellation cannot leak lines into a later run. - Emission no longer reads TraceCapture (cross-node ThreadStatic read that broke on thread switches and cache skew). QUARRY_TRACE gating moved to a FileEmitter ctor flag so the cached plan is never mutated - defining the symbol later still finds the lines. - Removed CallSiteTranslator's per-site trace log: the transform is per-site cached so the line was lost on warm runs; the failure text reaches traces via Clause.ErrorMessage in the retroactive LogSiteTrace. - TraceIncrementalCachingTests: trace comments survive a warm run after an unrelated-file edit, with TraceCapture explicitly cleared between runs to simulate the cross-thread loss mode (acceptance criterion 3).
…mbdaInnerSiteIds)
- The set is now an out param consumed by PipelineOrchestrator in the same call instead of a [ThreadStatic] with a consume-then-clear protocol. A cancellation between populate and clear could leave stale UniqueIds that the next run applied to its interceptor-site filter; since UniqueId is a location hash, a site at the same location could be silently excluded from generation (acceptance criterion 4 - impossible by construction now). - Unit test asserts the out-set contract and that runs carry no state over.
…ocs (#311) Under the carrier-only model there is no runtime query builder: builder methods are default-interface throw stubs, non-analyzable chains are QRY032 compile errors, and a clause that fails translation has its interceptor skipped so the call throws InvalidOperationException at runtime. - QRY001 description and QRY019 messageFormat/description now state that consequence instead of 'The original runtime method will be used instead'. - Updated the 4 QRY019-format comment references in CallSiteTranslator, the ShouldSkipNonTranslatableClause doc comment, ProjectionAnalyzer's 'triggers runtime fallback' comment, the generator README QRY001 row, and the analyzer-rules.md QRY019 contributor note. - QRY041's 'runtime ordinal discovery' text is a real, live mechanism and stays. v0.4.0 release notes quote the old text as history - untouched. - Found (not fixed): TypeMappingRegistry's TryConvert/TryConfigureParameter runtime-fallback path has no production callers - recorded for a separate issue.
- Error Propagation & QRY900 section rewritten: all error channels are now pipeline values (BindStageResult, TranslatedCallSite.PipelineError, deferred DiagnosticInfo); five QRY900 source paths enumerated including the unregistered-ID fallback. ThreadStatic-lifecycle paragraph and the 'single-threaded per compilation' claim removed. - Stage 3a diagram row, file tables (PipelineErrorBag -> BindStageResult, TraceCapture), DiagnosticInfo row, QRY048 registration note, and Key Design Decisions #2 (no cross-node ThreadStatic state) updated.
… TypeMappingRegistry removed (#311) Findings F1-F15 from the branch review (see review.md), 11A/3B/1D applied: - F4: unclaimed deferred diagnostics (file with no interceptor group, or no file path) are collected into a synthetic 'OrphanDiagnostics' group so they always report; llm.md caveat replaced; 2 unit tests. - F5: Stage 4 translate catch excludes OperationCanceledException - a cancellation can no longer become a sticky cached QRY900. - F6: ChainAnalyzer.Analyze diagnostics param is required; the three catch handlers can no longer silently swallow on a null list. - F1: deferred-descriptor miss path refactored into pure ResolveDeferredReport + unit tests for both branches. - F2: bind test hook supports '*'; new test proves QRY900 surfaces from a file whose every site fails bind (no interceptor group exists). - F8: TraceCapture.IsEmpty pins the cleared-after-run lifecycle (fails on pre-#311 code); trace test comments no longer overclaim. - F9/F10/F11: trivial-assertion removal, emitTraceComments fail-safe default, BindStageResult ctor null guards. - F3a: QRY001 and QRY019 raised Warning -> Error. Under the carrier-only model both mean the call site gets no interceptor and throws InvalidOperationException at runtime; a probe confirmed supported multi-hop variable chains analyze clean, so no false-positive shape is known. Doc tables updated. - F3b: dead runtime-fallback machinery removed - TypeMappingRegistry, ITypeMappingConverter, and their tests; TypeMapping ctor no longer registers; related doc text corrected. IDialectAwareTypeMapping stays (generated code calls ConfigureParameter directly). All 3397 + 146 + 201 tests green.
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.
Summary
Makes every diagnostic channel in the generator a value in the incremental pipeline instead of
[ThreadStatic]side-state, and makes the deferred-diagnostic registry fail loud instead of silently dropping unregistered IDs. All four issue findings addressed, plus a fifth occurrence of the silent-drop trap found during design (QRY063 emitted as a raw string, unregistered).Reason for Change
Issue #311 (multi-agent deep review, adversarially verified) identified one root theme: error/trace side-channels and the deferred-diagnostic registry could silently lose diagnostics, and the loss modes had already shipped three times (QRY048 in #304, then QRY900 and QRY063 found here). Bind-stage QRY900s were drained-and-discarded before reporting;
TraceCaptureandConsumedLambdaInnerSiteIdswere ThreadStatic state crossing pipeline-node (and thus thread/cache) boundaries; QRY001/QRY019 text still promised a runtime fallback that no longer exists.Impact
BindStageResultvalues to a dedicated report node — they surface as QRY900 even when a file's only site fails bind (previously: never reported).PipelineErrorBagis deleted.continue— silent drop). QRY900 and QRY063 are now registered; QRY063 fires for the first time.OrphanDiagnosticsgroup and still report..Trace()output is captured ontoAssembledPlan.TraceLinesinside the orchestrator (equality-excluded), so cached file groups keep their// [Trace]comments on incremental runs; the QUARRY_TRACE gate moved to aFileEmitterflag so cached plans are never mutated.OperationCanceledException,ConsumedLambdaInnerSiteIdsis returned fromAnalyzeinstead of stored ThreadStatic, andTraceCaptureclears in afinally.Acceptance criteria from #311: all four covered by tests (forced bind exception → QRY900; unregistered-ID fallback unit-tested — see Deviations for the skipped registry-scan test; warm-run trace persistence incl. a lifecycle assertion that fails on the old code; cancellation poisoning impossible by construction, pinned by the out-param contract test).
Plan items implemented as specified
s_deferredDescriptors, replace the silent miss path with a QRY900 report (ReportDeferredDiagnostic/ResolveDeferredReport).BindStageResultvalue pipeline, dedicated failure report node, both drains removed, ChainAnalyzer catches rerouted to deferred diagnostics,PipelineErrorBagdeleted, forced-bind-exception tests.TraceCapture; redundant Stage-4 trace log removed.llm.mderror-propagation section rewritten for the value-channel model.Deviations from plan implemented
OperationCanceledException(agreed in-flight): previously a cancelled bind would have been recorded as an error.Gaps in original plan implemented
Post-implementation review (15 findings, all classified; see below for the two that became scope):
GroupTranslatedIntoFiles.OperationCanceledExceptioninto the cachedPipelineError; fixed.InvalidOperationExceptionat runtime. A probe confirmed supported multi-hop variable chains analyze clean (no known false-positive shape).TypeMappingRegistry(TryConvert/TryConfigureParameterhad no production callers; the doc-claimed consumerNormalizeParameterValuedoes not exist),ITypeMappingConverter, and their tests.TypeMappingno longer registers into a registry.IDialectAwareTypeMappingis unchanged — generated code callsConfigureParameterdirectly.BindStageResultctor null guards,FileEmittertrace-comment default flipped to fail-safe,ChainAnalyzer.Analyzediagnosticsparameter made required.Performance Considerations
No hot-path changes. Stage 3 allocates one small wrapper per bound site (build-time only); trace capture moved, not added; the orphan-diagnostic scan is O(diagnostics) per orchestrator run.
Breaking Changes
InvalidOperationExceptionon execution); it now fails at compile time.TreatWarningsAsErrors.TypeMapping<TCustom,TDb>no longer implements the internalITypeMappingConverterand has no registering constructor — no public API surface removed.ChainAnalyzer.Analyzesignature: consumed-lambda-inner IDs out param;diagnosticsrequired.CallSiteBinder.Binderrors are carried byBindStageResult;PipelineErrorBagandTypeMappingRegistrydeleted.