Skip to content

fix: preserve incremental pipeline cancellation/cache stability - #117

Merged
2 commits merged into
mainfrom
fix/incremental-pipeline-cancellation-cache
May 9, 2026
Merged

fix: preserve incremental pipeline cancellation/cache stability#117
2 commits merged into
mainfrom
fix/incremental-pipeline-cancellation-cache

Conversation

@ANcpLua

@ANcpLua ANcpLua commented May 9, 2026

Copy link
Copy Markdown
Owner

Summary

  • preserves cancellation/caching behavior in incremental pipeline helpers
  • adds shared xUnit cancellation contract and generator error metadata
  • covers the incremental provider behavior with tests

Verification

  • dotnet test -c Release

Summary by CodeRabbit

  • New Features

    • Enhanced error information capture in incremental generators with structured error reporting.
  • Tests

    • Added comprehensive test coverage for error handling, generator extensions, and cancellation propagation behaviors.

@github-actions

github-actions Bot commented May 9, 2026

Copy link
Copy Markdown

@coderabbitai autofix

@coderabbitai

coderabbitai Bot commented May 9, 2026

Copy link
Copy Markdown

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

The PR introduces a new shared contract project (XunitCancellationShared) centralizing diagnostic metadata and cancellation-token detection logic. Analyzer and fixer projects now reference this contract instead of duplicating constants. Core utility extensions in IncrementalValuesProviderExtensions were refactored to represent generator selector failures as (Value, Error) tuples using a new GeneratorErrorInfo readonly record struct, enabling value-equatable exception snapshots for incremental caching. GroupBy now preserves deterministic key insertion order. OperationCanceledException is rethrown to abort pipelines; other exceptions are captured. Tests validate error-info capture, value equality, key ordering, and cancellation propagation.

🚥 Pre-merge checks | ✅ 8 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
No Null-Forgiving Operator Without Justification ⚠️ Warning Three null-forgiving operators found in modified files without inline comments: line 223 (text!), line 311 (x.Value!), and line 489 (x.Value!) in IncrementalValuesProviderExtensions.cs. Add inline comments (// ...) directly after each null-forgiving operator to justify suppression of null warnings.
✅ Passed checks (8 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Title follows conventional commits format (fix prefix), is under 72 chars (63 chars), has no trailing period, and accurately describes the main changes: cancellation preservation and cache stability improvements.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
No Datetime.Now/Utcnow ✅ Passed Scanned all 7 modified C# files. No DateTime.Now or DateTime.UtcNow usages found. netstandard2.0 polyfill discipline maintained.
No .Result/.Wait() Blocking Async ✅ Passed No blocking async operations found. Three .Result references are property accesses on ResultWithDiagnostics, not Task.Result calls. No .Wait() or .GetAwaiter().GetResult() present.
No Isourcegenerator ✅ Passed No ISourceGenerator implementations found. All generator classes in the PR use IIncrementalGenerator as required. Comprehensive scan of 219 C# files found zero ISourceGenerator references.
Sources Public Types Must Be Internal ✅ Passed src/ANcpLua.Roslyn.Utilities.Sources/ contains zero .cs files. Transformation to internal happens at build time, not in source.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@ghost
ghost enabled auto-merge (squash) May 9, 2026 10:16
@coderabbitai

coderabbitai Bot commented May 9, 2026

Copy link
Copy Markdown

Note

Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it.

Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found.

@ghost
ghost merged commit d559634 into main May 9, 2026
12 checks passed
@ghost
ghost deleted the fix/incremental-pipeline-cancellation-cache branch May 9, 2026 10:17

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 091d8f5f06

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +289 to 293
catch (OperationCanceledException)
{
// Cancellation must propagate so Roslyn can abort the pipeline.
throw;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Only rethrow cancellation tied to Roslyn token

This helper now rethrows every OperationCanceledException, which means selectors that throw OCE for reasons unrelated to the pipeline token (for example, a nested API using its own cancellation token) will abort generation and skip the SRE001 diagnostic path. That breaks the method’s “report exceptions and continue” behavior for a real class of failures; it should only propagate when the Roslyn cancellationToken is actually canceled (e.g., catch filter on cancellationToken.IsCancellationRequested or matching token).

Useful? React with 👍 / 👎.

This pull request was closed.
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