From 647886c7d818f2e15a9acc82dd67876d963c1761 Mon Sep 17 00:00:00 2001 From: Wouter Van Ranst Date: Sun, 12 Apr 2026 14:26:38 +0200 Subject: [PATCH 1/8] feat: add skills from https://github.com/Aaronontheweb/dotnet-skills --- .github/skills/crap-analysis/SKILL.md | 387 +++++++++++++ .github/skills/csharp-api-design/SKILL.md | 367 ++++++++++++ .../skills/csharp-coding-standards/SKILL.md | 320 +++++++++++ .../anti-patterns-and-reflection.md | 288 ++++++++++ .../composition-and-error-handling.md | 298 ++++++++++ .../performance-and-api-design.md | 346 ++++++++++++ .../value-objects-and-patterns.md | 247 +++++++++ .../csharp-type-design-performance/SKILL.md | 398 +++++++++++++ .github/skills/ilspy-decompile/SKILL.md | 102 ++++ .../SKILL.md | 343 ++++++++++++ .../advanced-patterns.md | 325 +++++++++++ .github/skills/package-management/SKILL.md | 459 +++++++++++++++ .github/skills/project-structure/SKILL.md | 521 ++++++++++++++++++ .github/skills/serialization/SKILL.md | 413 ++++++++++++++ .github/skills/slopwatch/SKILL.md | 318 +++++++++++ .github/skills/testcontainers/SKILL.md | 225 ++++++++ .../testcontainers/database-patterns.md | 201 +++++++ .../testcontainers/infrastructure-patterns.md | 387 +++++++++++++ 18 files changed, 5945 insertions(+) create mode 100644 .github/skills/crap-analysis/SKILL.md create mode 100644 .github/skills/csharp-api-design/SKILL.md create mode 100644 .github/skills/csharp-coding-standards/SKILL.md create mode 100644 .github/skills/csharp-coding-standards/anti-patterns-and-reflection.md create mode 100644 .github/skills/csharp-coding-standards/composition-and-error-handling.md create mode 100644 .github/skills/csharp-coding-standards/performance-and-api-design.md create mode 100644 .github/skills/csharp-coding-standards/value-objects-and-patterns.md create mode 100644 .github/skills/csharp-type-design-performance/SKILL.md create mode 100644 .github/skills/ilspy-decompile/SKILL.md create mode 100644 .github/skills/microsoft-extensions-configuration/SKILL.md create mode 100644 .github/skills/microsoft-extensions-configuration/advanced-patterns.md create mode 100644 .github/skills/package-management/SKILL.md create mode 100644 .github/skills/project-structure/SKILL.md create mode 100644 .github/skills/serialization/SKILL.md create mode 100644 .github/skills/slopwatch/SKILL.md create mode 100644 .github/skills/testcontainers/SKILL.md create mode 100644 .github/skills/testcontainers/database-patterns.md create mode 100644 .github/skills/testcontainers/infrastructure-patterns.md diff --git a/.github/skills/crap-analysis/SKILL.md b/.github/skills/crap-analysis/SKILL.md new file mode 100644 index 00000000..fa6d9ccf --- /dev/null +++ b/.github/skills/crap-analysis/SKILL.md @@ -0,0 +1,387 @@ +--- +name: crap-analysis +description: Analyze code coverage and CRAP (Change Risk Anti-Patterns) scores to identify high-risk code. Use OpenCover format with ReportGenerator for Risk Hotspots showing cyclomatic complexity and untested code paths. +invocable: true +--- + +# CRAP Score Analysis + +## When to Use This Skill + +Use this skill when: +- Evaluating code quality and test coverage before changes +- Identifying high-risk code that needs refactoring or testing +- Setting up coverage collection for a .NET project +- Prioritizing which code to test based on risk +- Establishing coverage thresholds for CI/CD pipelines + +--- + +## What is CRAP? + +**CRAP Score = Complexity x (1 - Coverage)^2** + +The CRAP (Change Risk Anti-Patterns) score combines cyclomatic complexity with test coverage to identify risky code. + +| CRAP Score | Risk Level | Action Required | +|------------|------------|-----------------| +| **< 5** | Low | Well-tested, maintainable code | +| **5-30** | Medium | Acceptable but watch complexity | +| **> 30** | High | Needs tests or refactoring | + +### Why CRAP Matters + +- **High complexity + low coverage = danger**: Code that's hard to understand AND untested is risky to modify +- **Complexity alone isn't enough**: A complex method with 100% coverage is safer than a simple method with 0% +- **Focuses effort**: Prioritize testing on complex code, not simple getters/setters + +### CRAP Score Examples + +| Method | Complexity | Coverage | Calculation | CRAP | +|--------|------------|----------|-------------|------| +| `GetUserId()` | 1 | 0% | 1 x (1 - 0)^2 | **1** | +| `ParseToken()` | 54 | 52% | 54 x (1 - 0.52)^2 | **12.4** | +| `ValidateForm()` | 20 | 0% | 20 x (1 - 0)^2 | **20** | +| `ProcessOrder()` | 45 | 20% | 45 x (1 - 0.20)^2 | **28.8** | +| `ImportData()` | 80 | 10% | 80 x (1 - 0.10)^2 | **64.8** | + +--- + +## Coverage Collection Setup + +### coverage.runsettings + +Create a `coverage.runsettings` file in your repository root. The **OpenCover format is required** for CRAP score calculation because it includes cyclomatic complexity metrics. + +```xml + + + + + + + + cobertura,opencover + + + [*.Tests]*,[*.Benchmark]*,[*.Migrations]* + + + Obsolete,GeneratedCodeAttribute,CompilerGeneratedAttribute,ExcludeFromCodeCoverageAttribute + + + **/obj/**/*,**/*.g.cs,**/*.designer.cs,**/*.razor.g.cs,**/*.razor.css.g.cs,**/Migrations/**/* + + + false + + + false + true + true + + + + + +``` + +### Key Configuration Options + +| Option | Purpose | +|--------|---------| +| `Format` | Must include `opencover` for complexity metrics | +| `Exclude` | Exclude test/benchmark assemblies by pattern | +| `ExcludeByAttribute` | Skip generated, obsolete, and explicitly excluded code (includes `ExcludeFromCodeCoverageAttribute`) | +| `ExcludeByFile` | Skip source-generated files, Blazor components, and migrations | +| `SkipAutoProps` | Don't count auto-properties as branches | + +--- + +## ReportGenerator Installation + +Install ReportGenerator as a local tool for generating HTML reports with Risk Hotspots. + +### Add to .config/dotnet-tools.json + +```json +{ + "version": 1, + "isRoot": true, + "tools": { + "dotnet-reportgenerator-globaltool": { + "version": "5.4.5", + "commands": ["reportgenerator"], + "rollForward": false + } + } +} +``` + +Then restore: + +```bash +dotnet tool restore +``` + +### Or Install Globally + +```bash +dotnet tool install --global dotnet-reportgenerator-globaltool +``` + +--- + +## Collecting Coverage + +### Run Tests with Coverage Collection + +```bash +# Clean previous results +rm -rf coverage/ TestResults/ + +# Run unit tests with coverage +dotnet test tests/MyApp.Tests.Unit \ + --settings coverage.runsettings \ + --collect:"XPlat Code Coverage" \ + --results-directory ./TestResults + +# Run integration tests (optional, adds to coverage) +dotnet test tests/MyApp.Tests.Integration \ + --settings coverage.runsettings \ + --collect:"XPlat Code Coverage" \ + --results-directory ./TestResults +``` + +### Generate HTML Report + +```bash +dotnet reportgenerator \ + -reports:"TestResults/**/coverage.opencover.xml" \ + -targetdir:"coverage" \ + -reporttypes:"Html;TextSummary;MarkdownSummaryGithub" +``` + +### Report Types + +| Type | Description | Output | +|------|-------------|--------| +| `Html` | Full interactive report | `coverage/index.html` | +| `TextSummary` | Plain text summary | `coverage/Summary.txt` | +| `MarkdownSummaryGithub` | GitHub-compatible markdown | `coverage/SummaryGithub.md` | +| `Badges` | SVG badges for README | `coverage/badge_*.svg` | +| `Cobertura` | Merged Cobertura XML | `coverage/Cobertura.xml` | + +--- + +## Reading the Report + +### Risk Hotspots Section + +The HTML report includes a **Risk Hotspots** section showing methods sorted by complexity: + +- **Cyclomatic Complexity**: Number of independent paths through code (if/else, switch cases, loops) +- **NPath Complexity**: Number of acyclic execution paths (exponential growth with nesting) +- **Crap Score**: Calculated from complexity and coverage + +### Interpreting Results + +``` +Risk Hotspots +───────────── +Method Complexity Coverage Crap Score +────────────────────────────────────────────────────────────────── +DataImporter.ParseRecord() 54 52% 12.4 +AuthService.ValidateToken() 32 0% 32.0 ← HIGH RISK +OrderProcessor.Calculate() 28 85% 1.3 +UserService.CreateUser() 15 100% 0.0 +``` + +**Action items:** +- `ValidateToken()` has CRAP > 30 with 0% coverage - **test immediately or refactor** +- `ParseRecord()` is complex but has decent coverage - acceptable +- `CreateUser()` and `Calculate()` are well-tested - safe to modify + +--- + +## Coverage Thresholds + +### Recommended Standards + +| Coverage Type | Target | Action | +|---------------|--------|--------| +| Line Coverage | > 80% | Good for most projects | +| Branch Coverage | > 60% | Catches conditional logic | +| CRAP Score | < 30 | Maximum for new code | + +### Configuring Thresholds + +Create `coverage.props` in your repository: + +```xml + + + + 80 + 60 + + +``` + +--- + +## CI/CD Integration + +### GitHub Actions + +```yaml +name: Coverage + +on: + pull_request: + branches: [main, dev] + +jobs: + coverage: + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v4 + + - name: Setup .NET + uses: actions/setup-dotnet@v4 + with: + dotnet-version: '9.0.x' + + - name: Restore tools + run: dotnet tool restore + + - name: Run tests with coverage + run: | + dotnet test \ + --settings coverage.runsettings \ + --collect:"XPlat Code Coverage" \ + --results-directory ./TestResults + + - name: Generate report + run: | + dotnet reportgenerator \ + -reports:"TestResults/**/coverage.opencover.xml" \ + -targetdir:"coverage" \ + -reporttypes:"Html;MarkdownSummaryGithub;Cobertura" + + - name: Upload coverage report + uses: actions/upload-artifact@v4 + with: + name: coverage-report + path: coverage/ + + - name: Add coverage to PR + uses: marocchino/sticky-pull-request-comment@v2 + with: + path: coverage/SummaryGithub.md +``` + +### Azure Pipelines + +```yaml +- task: DotNetCoreCLI@2 + displayName: 'Run tests with coverage' + inputs: + command: 'test' + arguments: '--settings coverage.runsettings --collect:"XPlat Code Coverage" --results-directory $(Build.SourcesDirectory)/TestResults' + +- task: DotNetCoreCLI@2 + displayName: 'Generate coverage report' + inputs: + command: 'custom' + custom: 'reportgenerator' + arguments: '-reports:"$(Build.SourcesDirectory)/TestResults/**/coverage.opencover.xml" -targetdir:"$(Build.SourcesDirectory)/coverage" -reporttypes:"HtmlInline_AzurePipelines;Cobertura"' + +- task: PublishCodeCoverageResults@2 + displayName: 'Publish coverage' + inputs: + codeCoverageTool: 'Cobertura' + summaryFileLocation: '$(Build.SourcesDirectory)/coverage/Cobertura.xml' +``` + +--- + +## Quick Reference + +### One-Liner Commands + +```bash +# Full analysis workflow +rm -rf coverage/ TestResults/ && \ +dotnet test --settings coverage.runsettings \ + --collect:"XPlat Code Coverage" \ + --results-directory ./TestResults && \ +dotnet reportgenerator \ + -reports:"TestResults/**/coverage.opencover.xml" \ + -targetdir:"coverage" \ + -reporttypes:"Html;TextSummary" + +# View summary +cat coverage/Summary.txt + +# Open HTML report (Linux) +xdg-open coverage/index.html + +# Open HTML report (macOS) +open coverage/index.html + +# Open HTML report (Windows) +start coverage/index.html +``` + +### Project Standards + +| Metric | New Code | Legacy Code | +|--------|----------|-------------| +| Line Coverage | 80%+ | 60%+ (improve gradually) | +| Branch Coverage | 60%+ | 40%+ (improve gradually) | +| Maximum CRAP | 30 | Document exceptions | +| High-risk methods | Must have tests | Add tests before modifying | + +--- + +## What Gets Excluded + +The recommended `coverage.runsettings` excludes: + +| Pattern | Reason | +|---------|--------| +| `[*.Tests]*` | Test assemblies aren't production code | +| `[*.Benchmark]*` | Benchmark projects | +| `[*.Migrations]*` | Database migrations (generated) | +| `GeneratedCodeAttribute` | Source generators | +| `CompilerGeneratedAttribute` | Compiler-generated code | +| `ExcludeFromCodeCoverageAttribute` | Explicit developer opt-out | +| `*.g.cs`, `*.designer.cs` | Generated files | +| `*.razor.g.cs` | Blazor component generated code | +| `*.razor.css.g.cs` | Blazor CSS isolation generated code | +| `**/Migrations/**/*` | EF Core migrations (auto-generated) | +| `SkipAutoProps` | Auto-properties (trivial branches) | + +--- + +## When to Update Thresholds + +**Lower thresholds temporarily for:** +- Legacy codebases being modernized (document in README) +- Generated code that can't be modified +- Third-party wrapper code + +**Never lower thresholds for:** +- "It's too hard to test" - refactor instead +- "We'll add tests later" - add them now +- New features - should meet standards from the start + +--- + +## Additional Resources + +- **Coverlet Documentation**: https://github.com/coverlet-coverage/coverlet +- **ReportGenerator**: https://github.com/danielpalme/ReportGenerator +- **CRAP Score Original Paper**: http://www.artima.com/weblogs/viewpost.jsp?thread=215899 diff --git a/.github/skills/csharp-api-design/SKILL.md b/.github/skills/csharp-api-design/SKILL.md new file mode 100644 index 00000000..ab8bf34e --- /dev/null +++ b/.github/skills/csharp-api-design/SKILL.md @@ -0,0 +1,367 @@ +--- +name: api-design +description: Design stable, compatible public APIs using extend-only design principles. Manage API compatibility, wire compatibility, and versioning for NuGet packages and distributed systems. +invocable: false +--- + +# Public API Design and Compatibility + +## When to Use This Skill + +Use this skill when: +- Designing public APIs for NuGet packages or libraries +- Making changes to existing public APIs +- Planning wire format changes for distributed systems +- Implementing versioning strategies +- Reviewing pull requests for breaking changes + +--- + +## The Three Types of Compatibility + +| Type | Definition | Scope | +|------|------------|-------| +| **API/Source** | Code compiles against newer version | Public method signatures, types | +| **Binary** | Compiled code runs against newer version | Assembly layout, method tokens | +| **Wire** | Serialized data readable by other versions | Network protocols, persistence formats | + +Breaking any of these creates upgrade friction for users. + +--- + +## Extend-Only Design + +The foundation of stable APIs: **never remove or modify, only extend**. + +### Three Pillars + +1. **Previous functionality is immutable** - Once released, behavior and signatures are locked +2. **New functionality through new constructs** - Add overloads, new types, opt-in features +3. **Removal only after deprecation period** - Years, not releases + +### Benefits + +- Old code continues working in new versions +- New and old pathways coexist +- Upgrades are non-breaking by default +- Users upgrade on their schedule + +**Resources:** +- [Extend-Only Design](https://aaronstannard.com/extend-only-design/) +- [OSS Compatibility Standards](https://aaronstannard.com/oss-compatibility-standards/) + +--- + +## API Change Guidelines + +### Safe Changes (Any Release) + +```csharp +// ADD new overloads with default parameters +public void Process(Order order, CancellationToken ct = default); + +// ADD new optional parameters to existing methods +public void Send(Message msg, Priority priority = Priority.Normal); + +// ADD new types, interfaces, enums +public interface IOrderValidator { } +public enum OrderStatus { Pending, Complete, Cancelled } + +// ADD new members to existing types +public class Order +{ + public DateTimeOffset? ShippedAt { get; init; } // NEW +} +``` + +### Unsafe Changes (Never or Major Version Only) + +```csharp +// REMOVE or RENAME public members +public void ProcessOrder(Order order); // Was: Process() + +// CHANGE parameter types or order +public void Process(int orderId); // Was: Process(Order order) + +// CHANGE return types +public Order? GetOrder(string id); // Was: public Order GetOrder() + +// CHANGE access modifiers +internal class OrderProcessor { } // Was: public + +// ADD required parameters without defaults +public void Process(Order order, ILogger logger); // Breaks callers! +``` + +### Deprecation Pattern + +```csharp +// Step 1: Mark as obsolete with version (any release) +[Obsolete("Obsolete since v1.5.0. Use ProcessAsync instead.")] +public void Process(Order order) { } + +// Step 2: Add new recommended API (same release) +public Task ProcessAsync(Order order, CancellationToken ct = default); + +// Step 3: Remove in next major version (v2.0+) +// Only after users have had time to migrate +``` + +--- + +## API Approval Testing + +Prevent accidental breaking changes with automated API surface testing. + +### Using ApiApprover + Verify + +```bash +dotnet add package PublicApiGenerator +dotnet add package Verify.Xunit +``` + +```csharp +[Fact] +public Task ApprovePublicApi() +{ + var api = typeof(MyLibrary.PublicClass).Assembly.GeneratePublicApi(); + return Verify(api); +} +``` + +Creates `ApprovePublicApi.verified.txt`: + +```csharp +namespace MyLibrary +{ + public class OrderProcessor + { + public OrderProcessor() { } + public void Process(Order order) { } + public Task ProcessAsync(Order order, CancellationToken ct = default) { } + } +} +``` + +**Any API change fails the test** - reviewer must explicitly approve changes. + +### PR Review Process + +1. PR includes changes to `*.verified.txt` files +2. Reviewers see exact API surface changes in diff +3. Breaking changes are immediately visible +4. Conscious decision required to approve + +--- + +## Wire Compatibility + +For distributed systems, serialized data must be readable across versions. + +### Requirements + +| Direction | Requirement | +|-----------|-------------| +| **Backward** | Old writers → New readers (current version reads old data) | +| **Forward** | New writers → Old readers (old version reads new data) | + +Both are required for zero-downtime rolling upgrades. + +### Safely Evolving Wire Formats + +**Phase 1: Add read-side support (opt-in)** + +```csharp +// New message type - readers deployed first +public sealed record HeartbeatV2( + Address From, + long SequenceNr, + long CreationTimeMs); // NEW field + +// Deserializer handles both old and new +public object Deserialize(byte[] data, string manifest) => manifest switch +{ + "Heartbeat" => DeserializeHeartbeatV1(data), // Old format + "HeartbeatV2" => DeserializeHeartbeatV2(data), // New format + _ => throw new NotSupportedException() +}; +``` + +**Phase 2: Enable write-side (opt-out, next minor version)** + +```csharp +// Config to enable new format (off by default initially) +akka.cluster.use-heartbeat-v2 = on +``` + +**Phase 3: Make default (future version)** + +After install base has absorbed read-side code. + +### Schema-Based Serialization + +Prefer schema-based formats over reflection-based: + +| Format | Type | Wire Compatibility | +|--------|------|-------------------| +| **Protocol Buffers** | Schema-based | Excellent - explicit field numbers | +| **MessagePack** | Schema-based | Good - with contracts | +| **System.Text.Json** | Schema-based (with source gen) | Good - explicit properties | +| Newtonsoft.Json | Reflection-based | Poor - type names in payload | +| BinaryFormatter | Reflection-based | Terrible - never use | + +See `dotnet/serialization` skill for details. + +--- + +## Encapsulation Patterns + +### Internal APIs + +Mark non-public APIs explicitly: + +```csharp +// Attribute for documentation +[InternalApi] +public class ActorSystemImpl { } + +// Namespace convention +namespace MyLibrary.Internal +{ + public class InternalHelper { } // Public for extensibility, not for users +} +``` + +Document clearly: + +> Types in `.Internal` namespaces or marked with `[InternalApi]` may change between any releases without notice. + +### Sealing Classes + +```csharp +// DO: Seal classes not designed for inheritance +public sealed class OrderProcessor { } + +// DON'T: Leave unsealed by accident +public class OrderProcessor { } // Users might inherit, blocking changes +``` + +### Interface Segregation + +```csharp +// DO: Small, focused interfaces +public interface IOrderReader +{ + Order? GetById(OrderId id); +} + +public interface IOrderWriter +{ + Task SaveAsync(Order order); +} + +// DON'T: Monolithic interfaces (can't add methods without breaking) +public interface IOrderRepository +{ + Order? GetById(OrderId id); + Task SaveAsync(Order order); + // Adding new methods breaks all implementations! +} +``` + +--- + +## Versioning Strategy + +### Semantic Versioning (Practical) + +| Version | Changes Allowed | +|---------|----------------| +| **Patch** (1.0.x) | Bug fixes, security patches | +| **Minor** (1.x.0) | New features, deprecations, obsolete removal | +| **Major** (x.0.0) | Breaking changes, old API removal | + +### Key Principles + +1. **No surprise breaks** - Even major versions should be announced and planned +2. **Extensions anytime** - New APIs can ship in any release +3. **Deprecate before remove** - `[Obsolete]` for at least one minor version +4. **Communicate timelines** - Users need to plan upgrades + +### Chesterton's Fence + +> Before removing or changing something, understand why it exists. + +Assume every public API is used by someone. If you want to change it: +1. Socialize the proposal on GitHub +2. Document migration path +3. Provide deprecation period +4. Ship in planned release + +--- + +## Pull Request Checklist + +When reviewing PRs that touch public APIs: + +- [ ] **No removed public members** (use `[Obsolete]` instead) +- [ ] **No changed signatures** (add overloads instead) +- [ ] **No new required parameters** (use defaults) +- [ ] **API approval test updated** (`.verified.txt` changes reviewed) +- [ ] **Wire format changes are opt-in** (read-side first) +- [ ] **Breaking changes documented** (release notes, migration guide) + +--- + +## Anti-Patterns + +### Breaking Changes Disguised as Fixes + +```csharp +// "Bug fix" that breaks users +public async Task GetOrderAsync(OrderId id) // Was sync! +{ + // "Fixed" to be async - but breaks all callers +} + +// Correct: Add new method, deprecate old +[Obsolete("Use GetOrderAsync instead")] +public Order GetOrder(OrderId id) => GetOrderAsync(id).Result; + +public async Task GetOrderAsync(OrderId id) { } +``` + +### Silent Behavior Changes + +```csharp +// Changing defaults breaks users who relied on old behavior +public void Configure(bool enableCaching = true) // Was: false! + +// Correct: New parameter with new name +public void Configure( + bool enableCaching = false, // Original default preserved + bool enableNewCaching = true) // New behavior opt-in +``` + +### Polymorphic Serialization + +```csharp +// AVOID: Type names in wire format +{ "$type": "MyApp.Order, MyApp", "Id": 123 } + +// Renaming Order class = wire break! + +// PREFER: Explicit discriminators +{ "type": "order", "id": 123 } +``` + +--- + +## Resources + +- [Making Public API Changes](https://getakka.net/community/contributing/api-changes-compatibility.html) +- [Wire Format Changes](https://getakka.net/community/contributing/wire-compatibility.html) +- [Extend-Only Design](https://aaronstannard.com/extend-only-design/) +- [OSS Compatibility Standards](https://aaronstannard.com/oss-compatibility-standards/) +- [Semantic Versioning](https://semver.org/) +- [PublicApiGenerator](https://github.com/PublicApiGenerator/PublicApiGenerator) diff --git a/.github/skills/csharp-coding-standards/SKILL.md b/.github/skills/csharp-coding-standards/SKILL.md new file mode 100644 index 00000000..89b804c4 --- /dev/null +++ b/.github/skills/csharp-coding-standards/SKILL.md @@ -0,0 +1,320 @@ +--- +name: modern-csharp-coding-standards +description: Write modern, high-performance C# code using records, pattern matching, value objects, async/await, Span/Memory, and best-practice API design patterns. Emphasizes functional-style programming with C# 12+ features. +invocable: false +--- + +# Modern C# Coding Standards + +## When to Use This Skill + +Use this skill when: +- Writing new C# code or refactoring existing code +- Designing public APIs for libraries or services +- Optimizing performance-critical code paths +- Implementing domain models with strong typing +- Building async/await-heavy applications +- Working with binary data, buffers, or high-throughput scenarios + +## Reference Files + +- [value-objects-and-patterns.md](value-objects-and-patterns.md): Full value object examples and pattern matching code +- [performance-and-api-design.md](performance-and-api-design.md): Span/Memory examples and API design principles +- [composition-and-error-handling.md](composition-and-error-handling.md): Composition over inheritance, Result type, testing patterns +- [anti-patterns-and-reflection.md](anti-patterns-and-reflection.md): Reflection avoidance and common anti-patterns + +## Core Principles + +1. **Immutability by Default** - Use `record` types and `init`-only properties +2. **Type Safety** - Leverage nullable reference types and value objects +3. **Modern Pattern Matching** - Use `switch` expressions and patterns extensively +4. **Async Everywhere** - Prefer async APIs with proper cancellation support +5. **Zero-Allocation Patterns** - Use `Span` and `Memory` for performance-critical code +6. **API Design** - Accept abstractions, return appropriately specific types +7. **Composition Over Inheritance** - Avoid abstract base classes, prefer composition +8. **Value Objects as Structs** - Use `readonly record struct` for value objects + +--- + +## Language Patterns + +### Records for Immutable Data (C# 9+) + +Use `record` types for DTOs, messages, events, and domain entities. + +```csharp +// Simple immutable DTO +public record CustomerDto(string Id, string Name, string Email); + +// Record with validation in constructor +public record EmailAddress +{ + public string Value { get; init; } + + public EmailAddress(string value) + { + if (string.IsNullOrWhiteSpace(value) || !value.Contains('@')) + throw new ArgumentException("Invalid email address", nameof(value)); + + Value = value; + } +} + +// Records with collections - use IReadOnlyList +public record ShoppingCart( + string CartId, + string CustomerId, + IReadOnlyList Items +) +{ + public decimal Total => Items.Sum(item => item.Price * item.Quantity); +} +``` + +**When to use `record class` vs `record struct`:** +- `record class` (default): Reference types, use for entities, aggregates, DTOs with multiple properties +- `record struct`: Value types, use for value objects (see next section) + +### Value Objects as readonly record struct + +Value objects should **always be `readonly record struct`** for performance and value semantics. Use explicit conversions, never implicit operators. + +```csharp +public readonly record struct OrderId(string Value) +{ + public OrderId(string value) : this( + !string.IsNullOrWhiteSpace(value) + ? value + : throw new ArgumentException("OrderId cannot be empty", nameof(value))) + { } + public override string ToString() => Value; +} + +public readonly record struct Money(decimal Amount, string Currency); +public readonly record struct CustomerId(Guid Value) +{ + public static CustomerId New() => new(Guid.NewGuid()); +} +``` + +See [value-objects-and-patterns.md](value-objects-and-patterns.md) for complete examples including multi-value objects, factory patterns, and the no-implicit-conversion rule. + +### Pattern Matching (C# 8-12) + +Use switch expressions, property patterns, relational patterns, and list patterns for cleaner code. + +```csharp +public decimal CalculateDiscount(Order order) => order switch +{ + { Total: > 1000m } => order.Total * 0.15m, + { Total: > 500m } => order.Total * 0.10m, + { Total: > 100m } => order.Total * 0.05m, + _ => 0m +}; +``` + +See [value-objects-and-patterns.md](value-objects-and-patterns.md) for full pattern matching examples. + +--- + +### Nullable Reference Types (C# 8+) + +Enable nullable reference types in your project and handle nulls explicitly. + +```csharp +// In .csproj + + enable + + +// Explicit nullability +public string? FindUserName(string userId) +{ + var user = _repository.Find(userId); + return user?.Name; +} + +// Pattern matching with null checks +public decimal GetDiscount(Customer? customer) => customer switch +{ + null => 0m, + { IsVip: true } => 0.20m, + { OrderCount: > 10 } => 0.10m, + _ => 0.05m +}; + +// Guard clauses with ArgumentNullException.ThrowIfNull (C# 11+) +public void ProcessOrder(Order? order) +{ + ArgumentNullException.ThrowIfNull(order); + // order is now non-nullable in this scope + Console.WriteLine(order.Id); +} +``` + +--- + +## Composition Over Inheritance + +**Avoid abstract base classes.** Use interfaces + composition. Use static helpers for shared logic. Use records with factory methods for variants. + +See [composition-and-error-handling.md](composition-and-error-handling.md) for full examples. + +--- + +## Performance Patterns + +### Async/Await Best Practices + +```csharp +// Async all the way - always accept CancellationToken +public async Task GetOrderAsync(string orderId, CancellationToken cancellationToken) +{ + var order = await _repository.GetAsync(orderId, cancellationToken); + return order; +} + +// ValueTask for frequently-called, often-synchronous methods +public ValueTask GetCachedOrderAsync(string orderId, CancellationToken cancellationToken) +{ + if (_cache.TryGetValue(orderId, out var order)) + return ValueTask.FromResult(order); + return GetFromDatabaseAsync(orderId, cancellationToken); +} + +// IAsyncEnumerable for streaming +public async IAsyncEnumerable StreamOrdersAsync( + string customerId, + [EnumeratorCancellation] CancellationToken cancellationToken = default) +{ + await foreach (var order in _repository.StreamAllAsync(cancellationToken)) + { + if (order.CustomerId == customerId) + yield return order; + } +} +``` + +**Key rules:** +- Always accept `CancellationToken` with `= default` +- Use `ConfigureAwait(false)` in library code +- Never block on async code (no `.Result` or `.Wait()`) +- Use linked CancellationTokenSource for timeouts + +### Span and Memory + +Use `Span` for synchronous zero-allocation operations, `Memory` for async, and `ArrayPool` for large temporary buffers. + +See [performance-and-api-design.md](performance-and-api-design.md) for complete Span/Memory examples and the API design section. + +--- + +## Error Handling: Result Type + +For expected errors, use `Result` instead of exceptions. Use exceptions only for unexpected/system errors. + +See [composition-and-error-handling.md](composition-and-error-handling.md) for the full Result type implementation and usage examples. + +--- + +## Avoid Reflection-Based Metaprogramming + +**Banned:** AutoMapper, Mapster, ExpressMapper. Use explicit mapping extension methods instead. Use `UnsafeAccessorAttribute` (.NET 8+) when you genuinely need private member access. + +See [anti-patterns-and-reflection.md](anti-patterns-and-reflection.md) for full guidance. + +--- + +## Code Organization + +```csharp +// File: Domain/Orders/Order.cs + +namespace MyApp.Domain.Orders; + +// 1. Primary domain type +public record Order( + OrderId Id, + CustomerId CustomerId, + Money Total, + OrderStatus Status, + IReadOnlyList Items +) +{ + public bool IsCompleted => Status is OrderStatus.Completed; + + public Result AddItem(OrderItem item) + { + if (Status is not OrderStatus.Draft) + return Result.Failure( + new OrderError("ORDER_NOT_DRAFT", "Can only add items to draft orders")); + + var newItems = Items.Append(item).ToList(); + var newTotal = new Money( + Items.Sum(i => i.Total.Amount) + item.Total.Amount, + Total.Currency); + + return Result.Success( + this with { Items = newItems, Total = newTotal }); + } +} + +// 2. Enums for state +public enum OrderStatus { Draft, Submitted, Processing, Completed, Cancelled } + +// 3. Related types +public record OrderItem(ProductId ProductId, Quantity Quantity, Money UnitPrice) +{ + public Money Total => new(UnitPrice.Amount * Quantity.Value, UnitPrice.Currency); +} + +// 4. Value objects +public readonly record struct OrderId(Guid Value) +{ + public static OrderId New() => new(Guid.NewGuid()); +} + +// 5. Errors +public readonly record struct OrderError(string Code, string Message); +``` + +--- + +## Best Practices Summary + +### DO's +- Use `record` for DTOs, messages, and domain entities +- Use `readonly record struct` for value objects +- Leverage pattern matching with `switch` expressions +- Enable and respect nullable reference types +- Use async/await for all I/O operations +- Accept `CancellationToken` in all async methods +- Use `Span` and `Memory` for high-performance scenarios +- Accept abstractions (`IEnumerable`, `IReadOnlyList`) +- Use `Result` for expected errors +- Pool buffers with `ArrayPool` for large allocations +- Prefer composition over inheritance + +### DON'Ts +- Don't use mutable classes when records work +- Don't use classes for value objects (use `readonly record struct`) +- Don't create deep inheritance hierarchies +- Don't ignore nullable reference type warnings +- Don't block on async code (`.Result`, `.Wait()`) +- Don't use `byte[]` when `Span` suffices +- Don't forget `CancellationToken` parameters +- Don't return mutable collections from APIs +- Don't throw exceptions for expected business errors +- Don't allocate large arrays repeatedly (use `ArrayPool`) + +See [anti-patterns-and-reflection.md](anti-patterns-and-reflection.md) for detailed anti-pattern examples. + +--- + +## Additional Resources + +- **C# Language Specification**: https://learn.microsoft.com/en-us/dotnet/csharp/ +- **Pattern Matching**: https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/functional/pattern-matching +- **Span and Memory**: https://learn.microsoft.com/en-us/dotnet/standard/memory-and-spans/ +- **Async Best Practices**: https://learn.microsoft.com/en-us/archive/msdn-magazine/2013/march/async-await-best-practices-in-asynchronous-programming +- **.NET Performance Tips**: https://learn.microsoft.com/en-us/dotnet/framework/performance/ diff --git a/.github/skills/csharp-coding-standards/anti-patterns-and-reflection.md b/.github/skills/csharp-coding-standards/anti-patterns-and-reflection.md new file mode 100644 index 00000000..f5ffc0b4 --- /dev/null +++ b/.github/skills/csharp-coding-standards/anti-patterns-and-reflection.md @@ -0,0 +1,288 @@ +# Anti-Patterns and Reflection Avoidance + +Guidelines on avoiding reflection-based metaprogramming and common C# anti-patterns. + +## Contents + +- [Avoid Reflection-Based Metaprogramming](#avoid-reflection-based-metaprogramming) +- [UnsafeAccessorAttribute (.NET 8+)](#unsafeaccessorattribute-net-8) +- [Anti-Patterns to Avoid](#anti-patterns-to-avoid) + +## Avoid Reflection-Based Metaprogramming + +**Prefer statically-typed, explicit code over reflection-based "magic" libraries.** + +Reflection-based libraries like AutoMapper trade compile-time safety for convenience. When mappings break, you find out at runtime (or worse, in production) instead of at compile time. + +### Banned Libraries + +| Library | Problem | +|---------|---------| +| **AutoMapper** | Reflection magic, hidden mappings, runtime failures, hard to debug | +| **Mapster** | Same issues as AutoMapper | +| **ExpressMapper** | Same issues | + +### Why Reflection Mapping Fails + +```csharp +// With AutoMapper - compiles fine, fails at runtime +public record UserDto(string Id, string Name, string Email); +public record UserEntity(Guid Id, string FullName, string EmailAddress); + +// This mapping silently produces garbage: +// - Id: string vs Guid mismatch +// - Name vs FullName: no match, null/default +// - Email vs EmailAddress: no match, null/default +var dto = _mapper.Map(entity); // Compiles! Breaks at runtime. +``` + +### Use Explicit Mapping Methods Instead + +```csharp +// Extension method - compile-time checked, easy to find, easy to debug +public static class UserMappings +{ + public static UserDto ToDto(this UserEntity entity) => new( + Id: entity.Id.ToString(), + Name: entity.FullName, + Email: entity.EmailAddress); + + public static UserEntity ToEntity(this CreateUserRequest request) => new( + Id: Guid.NewGuid(), + FullName: request.Name, + EmailAddress: request.Email); +} + +// Usage - explicit and traceable +var dto = entity.ToDto(); +var entity = request.ToEntity(); +``` + +### Benefits of Explicit Mappings + +| Aspect | AutoMapper | Explicit Methods | +|--------|------------|------------------| +| **Compile-time safety** | No - runtime errors | Yes - compiler catches mismatches | +| **Discoverability** | Hidden in profiles | "Go to Definition" works | +| **Debugging** | Black box | Step through code | +| **Refactoring** | Rename breaks silently | IDE renames correctly | +| **Performance** | Reflection overhead | Direct property access | +| **Testing** | Need integration tests | Simple unit tests | + +### Complex Mappings + +For complex transformations, explicit code is even more valuable: + +```csharp +public static OrderSummaryDto ToSummary(this Order order) => new( + OrderId: order.Id.Value.ToString(), + CustomerName: order.Customer.FullName, + ItemCount: order.Items.Count, + Total: order.Items.Sum(i => i.Quantity * i.UnitPrice), + Status: order.Status switch + { + OrderStatus.Pending => "Awaiting Payment", + OrderStatus.Paid => "Processing", + OrderStatus.Shipped => "On the Way", + OrderStatus.Delivered => "Completed", + _ => "Unknown" + }, + FormattedDate: order.CreatedAt.ToString("MMMM d, yyyy")); +``` + +This is: +- **Readable**: Anyone can understand the transformation +- **Debuggable**: Set a breakpoint, inspect values +- **Testable**: Pass an Order, assert on the result +- **Refactorable**: Change a property name, compiler tells you everywhere it's used + +### When Reflection is Acceptable + +Reflection has legitimate uses, but mapping DTOs isn't one of them: + +| Use Case | Acceptable? | +|----------|-------------| +| Serialization (System.Text.Json, Newtonsoft) | Yes - well-tested, source generators available | +| Dependency injection container | Yes - framework infrastructure | +| ORM entity mapping (EF Core) | Yes - necessary for database abstraction | +| Test fixtures and builders | Sometimes - for convenience in tests only | +| **DTO/domain object mapping** | **No - use explicit methods** | + +## UnsafeAccessorAttribute (.NET 8+) + +When you genuinely need to access private or internal members (serializers, test helpers, framework code), use `UnsafeAccessorAttribute` instead of traditional reflection. It provides **zero-overhead, AOT-compatible** member access. + +```csharp +// AVOID: Traditional reflection - slow, allocates, breaks AOT +var field = typeof(Order).GetField("_status", BindingFlags.NonPublic | BindingFlags.Instance); +var status = (OrderStatus)field!.GetValue(order)!; + +// PREFER: UnsafeAccessor - zero overhead, AOT-compatible +[UnsafeAccessor(UnsafeAccessorKind.Field, Name = "_status")] +static extern ref OrderStatus GetStatusField(Order order); + +var status = GetStatusField(order); // Direct access, no reflection +``` + +**Supported accessor kinds:** + +```csharp +// Private field access +[UnsafeAccessor(UnsafeAccessorKind.Field, Name = "_items")] +static extern ref List GetItemsField(Order order); + +// Private method access +[UnsafeAccessor(UnsafeAccessorKind.Method, Name = "Recalculate")] +static extern void CallRecalculate(Order order); + +// Private static field +[UnsafeAccessor(UnsafeAccessorKind.StaticField, Name = "_instanceCount")] +static extern ref int GetInstanceCount(Order order); + +// Private constructor +[UnsafeAccessor(UnsafeAccessorKind.Constructor)] +static extern Order CreateOrder(OrderId id, CustomerId customerId); +``` + +**Why UnsafeAccessor over reflection:** + +| Aspect | Reflection | UnsafeAccessor | +|--------|------------|----------------| +| Performance | Slow (100-1000x) | Zero overhead | +| AOT compatible | No | Yes | +| Allocations | Yes (boxing, arrays) | None | +| Compile-time checked | No | Partially (signature) | + +**Use cases:** +- Serializers accessing private backing fields +- Test helpers verifying internal state +- Framework code that needs to bypass visibility + +**Resources:** +- [A new way of doing reflection with .NET 8](https://steven-giesel.com/blogPost/05ecdd16-8dc4-490f-b1cf-780c994346a4) +- [Accessing private members without reflection in .NET 8.0](https://www.strathweb.com/2023/10/accessing-private-members-without-reflection-in-net-8-0/) +- [Modern .NET Reflection with UnsafeAccessor](https://blog.ndepend.com/modern-net-reflection-with-unsafeaccessor/) + +## Anti-Patterns to Avoid + +### Don't: Use mutable DTOs + +```csharp +// BAD: Mutable DTO +public class CustomerDto +{ + public string Id { get; set; } + public string Name { get; set; } +} + +// GOOD: Immutable record +public record CustomerDto(string Id, string Name); +``` + +### Don't: Use classes for value objects + +```csharp +// BAD: Value object as class +public class OrderId +{ + public string Value { get; } + public OrderId(string value) => Value = value; +} + +// GOOD: Value object as readonly record struct +public readonly record struct OrderId(string Value); +``` + +### Don't: Create deep inheritance hierarchies + +```csharp +// BAD: Deep inheritance +public abstract class Entity { } +public abstract class AggregateRoot : Entity { } +public abstract class Order : AggregateRoot { } +public class CustomerOrder : Order { } + +// GOOD: Flat structure with composition +public interface IEntity +{ + Guid Id { get; } +} + +public record Order(OrderId Id, CustomerId CustomerId, Money Total) : IEntity +{ + Guid IEntity.Id => Id.Value; +} +``` + +### Don't: Return List when you mean IReadOnlyList + +```csharp +// BAD: Exposes internal list for modification +public List GetOrders() => _orders; + +// GOOD: Returns read-only view +public IReadOnlyList GetOrders() => _orders; +``` + +### Don't: Use byte[] when ReadOnlySpan works + +```csharp +// BAD: Allocates array on every call +public byte[] GetHeader() +{ + var header = new byte[64]; + // Fill header + return header; +} + +// GOOD: Zero allocation with Span +public void GetHeader(Span destination) +{ + if (destination.Length < 64) + throw new ArgumentException("Buffer too small"); + + // Fill header directly into caller's buffer +} +``` + +### Don't: Forget CancellationToken in async methods + +```csharp +// BAD: No cancellation support +public async Task GetOrderAsync(OrderId id) +{ + return await _repository.GetAsync(id); +} + +// GOOD: Cancellation support +public async Task GetOrderAsync( + OrderId id, + CancellationToken cancellationToken = default) +{ + return await _repository.GetAsync(id, cancellationToken); +} +``` + +### Don't: Block on async code + +```csharp +// BAD: Deadlock risk! +public Order GetOrder(OrderId id) +{ + return GetOrderAsync(id).Result; +} + +// BAD: Also deadlock risk! +public Order GetOrder(OrderId id) +{ + return GetOrderAsync(id).GetAwaiter().GetResult(); +} + +// GOOD: Async all the way +public async Task GetOrderAsync( + OrderId id, + CancellationToken cancellationToken) +{ + return await _repository.GetAsync(id, cancellationToken); +} +``` diff --git a/.github/skills/csharp-coding-standards/composition-and-error-handling.md b/.github/skills/csharp-coding-standards/composition-and-error-handling.md new file mode 100644 index 00000000..25d19be2 --- /dev/null +++ b/.github/skills/csharp-coding-standards/composition-and-error-handling.md @@ -0,0 +1,298 @@ +# Composition and Error Handling + +Composition over inheritance, Result type pattern, and testing patterns for modern C#. + +## Contents + +- [Composition Over Inheritance](#composition-over-inheritance) +- [Result Type Pattern (Railway-Oriented Programming)](#result-type-pattern-railway-oriented-programming) +- [Testing Patterns](#testing-patterns) + +## Composition Over Inheritance + +**Avoid abstract base classes and inheritance hierarchies.** Use composition and interfaces instead. + +```csharp +// BAD: Abstract base class hierarchy +public abstract class PaymentProcessor +{ + public abstract Task ProcessAsync(Money amount); + + protected async Task ValidateAsync(Money amount) + { + // Shared validation logic + return amount.Amount > 0; + } +} + +public class CreditCardProcessor : PaymentProcessor +{ + public override async Task ProcessAsync(Money amount) + { + await ValidateAsync(amount); + // Process credit card... + } +} + +// GOOD: Composition with interfaces +public interface IPaymentProcessor +{ + Task ProcessAsync(Money amount, CancellationToken cancellationToken); +} + +public interface IPaymentValidator +{ + Task ValidateAsync(Money amount, CancellationToken cancellationToken); +} + +// Concrete implementations compose validators +public sealed class CreditCardProcessor : IPaymentProcessor +{ + private readonly IPaymentValidator _validator; + private readonly ICreditCardGateway _gateway; + + public CreditCardProcessor(IPaymentValidator validator, ICreditCardGateway gateway) + { + _validator = validator; + _gateway = gateway; + } + + public async Task ProcessAsync(Money amount, CancellationToken cancellationToken) + { + var validation = await _validator.ValidateAsync(amount, cancellationToken); + if (!validation.IsValid) + return PaymentResult.Failed(validation.Error); + + return await _gateway.ChargeAsync(amount, cancellationToken); + } +} + +// GOOD: Static helper classes for shared logic (no inheritance) +public static class PaymentValidation +{ + public static ValidationResult ValidateAmount(Money amount) + { + if (amount.Amount <= 0) + return ValidationResult.Invalid("Amount must be positive"); + + if (amount.Amount > 10000m) + return ValidationResult.Invalid("Amount exceeds maximum"); + + return ValidationResult.Valid(); + } +} + +// GOOD: Records for modeling variants (not inheritance) +public enum PaymentType { CreditCard, BankTransfer, Cash } + +public record PaymentMethod +{ + public PaymentType Type { get; init; } + public string? Last4 { get; init; } // For credit cards + public string? AccountNumber { get; init; } // For bank transfers + + public static PaymentMethod CreditCard(string last4) => new() + { + Type = PaymentType.CreditCard, + Last4 = last4 + }; + + public static PaymentMethod BankTransfer(string accountNumber) => new() + { + Type = PaymentType.BankTransfer, + AccountNumber = accountNumber + }; + + public static PaymentMethod Cash() => new() { Type = PaymentType.Cash }; +} +``` + +**When inheritance is acceptable:** +- Framework requirements (e.g., `ControllerBase` in ASP.NET Core) +- Library integration (e.g., custom exceptions inheriting from `Exception`) +- **These should be rare cases in your application code** + +## Result Type Pattern (Railway-Oriented Programming) + +For expected errors, use a `Result` type instead of exceptions. + +```csharp +// Simple Result type as readonly record struct +public readonly record struct Result +{ + private readonly TValue? _value; + private readonly TError? _error; + private readonly bool _isSuccess; + + private Result(TValue value) + { + _value = value; + _error = default; + _isSuccess = true; + } + + private Result(TError error) + { + _value = default; + _error = error; + _isSuccess = false; + } + + public bool IsSuccess => _isSuccess; + public bool IsFailure => !_isSuccess; + + public TValue Value => _isSuccess + ? _value! + : throw new InvalidOperationException("Cannot access Value of a failed result"); + + public TError Error => !_isSuccess + ? _error! + : throw new InvalidOperationException("Cannot access Error of a successful result"); + + public static Result Success(TValue value) => new(value); + public static Result Failure(TError error) => new(error); + + public Result Map(Func mapper) + => _isSuccess + ? Result.Success(mapper(_value!)) + : Result.Failure(_error!); + + public Result Bind(Func> binder) + => _isSuccess ? binder(_value!) : Result.Failure(_error!); + + public TValue GetValueOr(TValue defaultValue) + => _isSuccess ? _value! : defaultValue; + + public TResult Match( + Func onSuccess, + Func onFailure) + => _isSuccess ? onSuccess(_value!) : onFailure(_error!); +} + +// Error type as readonly record struct +public readonly record struct OrderError(string Code, string Message); + +// Usage example +public sealed class OrderService(IOrderRepository repository) +{ + public async Task> CreateOrderAsync( + CreateOrderRequest request, + CancellationToken cancellationToken) + { + // Validate + var validationResult = ValidateRequest(request); + if (validationResult.IsFailure) + return Result.Failure(validationResult.Error); + + // Check inventory + var inventoryResult = await CheckInventoryAsync(request.Items, cancellationToken); + if (inventoryResult.IsFailure) + return Result.Failure(inventoryResult.Error); + + // Create order + var order = new Order( + OrderId.New(), + new CustomerId(request.CustomerId), + request.Items); + + await repository.SaveAsync(order, cancellationToken); + + return Result.Success(order); + } + + // Pattern matching on Result + public IActionResult MapToActionResult(Result result) + { + return result.Match( + onSuccess: order => new OkObjectResult(order), + onFailure: error => error.Code switch + { + "VALIDATION_ERROR" => new BadRequestObjectResult(error.Message), + "INSUFFICIENT_INVENTORY" => new ConflictObjectResult(error.Message), + "NOT_FOUND" => new NotFoundObjectResult(error.Message), + _ => new ObjectResult(error.Message) { StatusCode = 500 } + } + ); + } +} +``` + +**When to use Result vs Exceptions:** +- **Use Result**: Expected errors (validation, business rules, not found) +- **Use Exceptions**: Unexpected errors (network failures, system errors, programming bugs) + +## Testing Patterns + +```csharp +// Use record for test data builders +public record OrderBuilder +{ + public OrderId Id { get; init; } = OrderId.New(); + public CustomerId CustomerId { get; init; } = CustomerId.New(); + public Money Total { get; init; } = new Money(100m, "USD"); + public IReadOnlyList Items { get; init; } = Array.Empty(); + + public Order Build() => new(Id, CustomerId, Total, Items); +} + +// Use 'with' expression for test variations +[Fact] +public void CalculateDiscount_LargeOrder_AppliesCorrectDiscount() +{ + // Arrange + var baseOrder = new OrderBuilder().Build(); + var largeOrder = baseOrder with + { + Total = new Money(1500m, "USD") + }; + + // Act + var discount = _service.CalculateDiscount(largeOrder); + + // Assert + discount.Should().Be(new Money(225m, "USD")); // 15% of 1500 +} + +// Span-based testing +[Theory] +[InlineData("ORD-12345", true)] +[InlineData("INVALID", false)] +public void TryParseOrderId_VariousInputs_ReturnsExpectedResult( + string input, + bool expected) +{ + // Act + var result = OrderIdParser.TryParse(input.AsSpan(), out var orderId); + + // Assert + result.Should().Be(expected); +} + +// Testing with value objects +[Fact] +public void Money_Add_SameCurrency_ReturnsSum() +{ + // Arrange + var money1 = new Money(100m, "USD"); + var money2 = new Money(50m, "USD"); + + // Act + var result = money1.Add(money2); + + // Assert + result.Should().Be(new Money(150m, "USD")); +} + +[Fact] +public void Money_Add_DifferentCurrency_ThrowsException() +{ + // Arrange + var usd = new Money(100m, "USD"); + var eur = new Money(50m, "EUR"); + + // Act & Assert + var act = () => usd.Add(eur); + act.Should().Throw() + .WithMessage("*different currencies*"); +} +``` diff --git a/.github/skills/csharp-coding-standards/performance-and-api-design.md b/.github/skills/csharp-coding-standards/performance-and-api-design.md new file mode 100644 index 00000000..daf3a285 --- /dev/null +++ b/.github/skills/csharp-coding-standards/performance-and-api-design.md @@ -0,0 +1,346 @@ +# Performance and API Design Patterns + +Zero-allocation patterns with Span/Memory and API design principles for accepting and returning the right types. + +## Contents + +- [Span and Memory for Zero-Allocation Code](#spant-and-memoryt-for-zero-allocation-code) +- [API Design Principles](#api-design-principles) +- [Method Signatures Best Practices](#method-signatures-best-practices) + +## Span and Memory for Zero-Allocation Code + +Use `Span` and `Memory` instead of `byte[]` or `string` for performance-critical code. + +```csharp +// Span for synchronous, zero-allocation operations +public int ParseOrderId(ReadOnlySpan input) +{ + // Work with data without allocations + if (!input.StartsWith("ORD-")) + throw new FormatException("Invalid order ID format"); + + var numberPart = input.Slice(4); + return int.Parse(numberPart); +} + +// stackalloc with Span +public void FormatMessage() +{ + Span buffer = stackalloc char[256]; + var written = FormatInto(buffer); + var message = new string(buffer.Slice(0, written)); +} + +// SkipLocalsInit with stackalloc - skips zero-initialization for performance +// By default, .NET zero-initializes all locals (.locals init flag). This can have +// measurable overhead with stackalloc. Use [SkipLocalsInit] when: +// - You write to the buffer before reading (like FormatInto below) +// - Profiling shows zero-init as a bottleneck +// WARNING: Reading before writing returns garbage data +// Requires: true in .csproj +// See: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/attributes/general#skiplocalsinit-attribute +using System.Runtime.CompilerServices; +[SkipLocalsInit] +public void FormatMessage() +{ + Span buffer = stackalloc char[256]; + var written = FormatInto(buffer); + var message = new string(buffer.Slice(0, written)); +} + +// Memory for async operations (Span can't cross await) +public async Task ReadDataAsync( + Memory buffer, + CancellationToken cancellationToken) +{ + return await _stream.ReadAsync(buffer, cancellationToken); +} + +// String manipulation with Span to avoid allocations +public bool TryParseKeyValue(ReadOnlySpan line, out string key, out string value) +{ + key = string.Empty; + value = string.Empty; + + int colonIndex = line.IndexOf(':'); + if (colonIndex == -1) + return false; + + // Only allocate strings once we know the format is valid + key = new string(line.Slice(0, colonIndex).Trim()); + value = new string(line.Slice(colonIndex + 1).Trim()); + return true; +} + +// ArrayPool for temporary large buffers +public async Task ProcessLargeFileAsync( + Stream stream, + CancellationToken cancellationToken) +{ + var buffer = ArrayPool.Shared.Rent(8192); + try + { + int bytesRead; + while ((bytesRead = await stream.ReadAsync(buffer.AsMemory(), cancellationToken)) > 0) + { + ProcessChunk(buffer.AsSpan(0, bytesRead)); + } + } + finally + { + ArrayPool.Shared.Return(buffer); + } +} + +// Hybrid buffer pattern for transient UTF-8 work. See caveats of SkipLocalsInit in the corresponding section. + +[SkipLocalsInit] +static short GenerateHashCode(string? key) +{ + if (key is null) return 0; + + const int StackLimit = 256; + + var enc = Encoding.UTF8; + var max = enc.GetMaxByteCount(key.Length); + + byte[]? rented = null; + Span buf = max <= StackLimit + ? stackalloc byte[StackLimit] + : (rented = ArrayPool.Shared.Rent(max)); + + try + { + var written = enc.GetBytes(key.AsSpan(), buf); + ComputeHash(buf[..written], out var h1, out var h2); + return unchecked((short)(h1 ^ h2)); + } + finally + { + if (rented is not null) ArrayPool.Shared.Return(rented); + } +} + +// Span-based parsing without substring allocations +public static (string Protocol, string Host, int Port) ParseUrl(ReadOnlySpan url) +{ + var protocolEnd = url.IndexOf("://"); + var protocol = new string(url.Slice(0, protocolEnd)); + + var afterProtocol = url.Slice(protocolEnd + 3); + var portStart = afterProtocol.IndexOf(':'); + + var host = new string(afterProtocol.Slice(0, portStart)); + var portSpan = afterProtocol.Slice(portStart + 1); + var port = int.Parse(portSpan); + + return (protocol, host, port); +} + +// Writing data to Span +public bool TryFormatOrderId(int orderId, Span destination, out int charsWritten) +{ + const string prefix = "ORD-"; + + if (destination.Length < prefix.Length + 10) + { + charsWritten = 0; + return false; + } + + prefix.AsSpan().CopyTo(destination); + var numberWritten = orderId.TryFormat( + destination.Slice(prefix.Length), + out var numberChars); + + charsWritten = prefix.Length + numberChars; + return numberWritten; +} +``` + +**When to use what:** + +| Type | Use Case | +|------|----------| +| `Span` | Synchronous operations, stack-allocated buffers, slicing without allocation | +| `ReadOnlySpan` | Read-only views, method parameters for data you won't modify | +| `Memory` | Async operations (Span can't cross await boundaries) | +| `ReadOnlyMemory` | Read-only async operations | +| `byte[]` | When you need to store data long-term or pass to APIs requiring arrays | +| `ArrayPool` | Large temporary buffers (>1KB) to avoid GC pressure | + +## API Design Principles + +### Accept Abstractions, Return Appropriately Specific + +**For Parameters (Accept):** + +```csharp +// Accept IEnumerable if you only iterate once +public decimal CalculateTotal(IEnumerable items) +{ + return items.Sum(item => item.Price * item.Quantity); +} + +// Accept IReadOnlyCollection if you need Count +public bool HasMinimumItems(IReadOnlyCollection items, int minimum) +{ + return items.Count >= minimum; +} + +// Accept IReadOnlyList if you need indexing +public OrderItem GetMiddleItem(IReadOnlyList items) +{ + if (items.Count == 0) + throw new ArgumentException("List cannot be empty"); + + return items[items.Count / 2]; // Indexed access +} + +// Accept ReadOnlySpan for high-performance, zero-allocation APIs +public int Sum(ReadOnlySpan numbers) +{ + int total = 0; + foreach (var num in numbers) + total += num; + return total; +} + +// Accept IAsyncEnumerable for async streaming +public async Task CountItemsAsync( + IAsyncEnumerable orders, + CancellationToken cancellationToken) +{ + int count = 0; + await foreach (var order in orders.WithCancellation(cancellationToken)) + count++; + return count; +} +``` + +**For Return Types:** + +```csharp +// Return IEnumerable for lazy/deferred execution +public IEnumerable GetOrdersLazy(string customerId) +{ + foreach (var order in _repository.Query()) + { + if (order.CustomerId == customerId) + yield return order; // Lazy evaluation + } +} + +// Return IReadOnlyList for materialized, immutable collections +public IReadOnlyList GetOrders(string customerId) +{ + return _repository + .Query() + .Where(o => o.CustomerId == customerId) + .ToList(); // Materialized +} + +// Return concrete types when callers need mutation +public List GetMutableOrders(string customerId) +{ + // Explicitly allow mutation by returning List + return _repository + .Query() + .Where(o => o.CustomerId == customerId) + .ToList(); +} + +// Return IAsyncEnumerable for async streaming +public async IAsyncEnumerable StreamOrdersAsync( + string customerId, + [EnumeratorCancellation] CancellationToken cancellationToken = default) +{ + await foreach (var order in _repository.StreamAllAsync(cancellationToken)) + { + if (order.CustomerId == customerId) + yield return order; + } +} + +// Return arrays for interop or when caller expects array +public byte[] SerializeOrder(Order order) +{ + // Binary serialization - byte[] is appropriate here + return MessagePackSerializer.Serialize(order); +} +``` + +**Summary Table:** + +| Scenario | Accept | Return | +|----------|--------|--------| +| Only iterate once | `IEnumerable` | `IEnumerable` (if lazy) | +| Need count | `IReadOnlyCollection` | `IReadOnlyCollection` | +| Need indexing | `IReadOnlyList` | `IReadOnlyList` | +| High-performance, sync | `ReadOnlySpan` | `Span` (rarely) | +| Async streaming | `IAsyncEnumerable` | `IAsyncEnumerable` | +| Caller needs mutation | - | `List`, `T[]` | + +## Method Signatures Best Practices + +```csharp +// Complete async method signature +public async Task> CreateOrderAsync( + CreateOrderRequest request, + CancellationToken cancellationToken = default) +{ + // Implementation +} + +// Optional parameters at the end +public async Task> GetOrdersAsync( + string customerId, + DateTime? startDate = null, + DateTime? endDate = null, + CancellationToken cancellationToken = default) +{ + // Implementation +} + +// Use record for multiple related parameters +public record SearchOrdersRequest( + string? CustomerId, + DateTime? StartDate, + DateTime? EndDate, + OrderStatus? Status, + int PageSize = 20, + int PageNumber = 1 +); + +public async Task> SearchOrdersAsync( + SearchOrdersRequest request, + CancellationToken cancellationToken = default) +{ + // Implementation +} + +// Primary constructors (C# 12+) for simple classes +public sealed class OrderService(IOrderRepository repository, ILogger logger) +{ + public async Task GetOrderAsync(OrderId orderId, CancellationToken cancellationToken) + { + logger.LogInformation("Fetching order {OrderId}", orderId); + return await repository.GetAsync(orderId, cancellationToken); + } +} + +// Options pattern for complex configuration +public sealed class EmailServiceOptions +{ + public required string SmtpHost { get; init; } + public int SmtpPort { get; init; } = 587; + public bool UseSsl { get; init; } = true; + public TimeSpan Timeout { get; init; } = TimeSpan.FromSeconds(30); +} + +public sealed class EmailService(IOptions options) +{ + private readonly EmailServiceOptions _options = options.Value; +} +``` diff --git a/.github/skills/csharp-coding-standards/value-objects-and-patterns.md b/.github/skills/csharp-coding-standards/value-objects-and-patterns.md new file mode 100644 index 00000000..cb96f9ed --- /dev/null +++ b/.github/skills/csharp-coding-standards/value-objects-and-patterns.md @@ -0,0 +1,247 @@ +# Value Objects and Pattern Matching + +Full code examples for value objects and pattern matching in modern C#. + +## Contents + +- [Value Objects as readonly record struct](#value-objects-as-readonly-record-struct) +- [No Implicit Conversions](#no-implicit-conversions) +- [Pattern Matching (C# 8-12)](#pattern-matching-c-8-12) + +## Value Objects as readonly record struct + +Value objects should **always be `readonly record struct`** for performance and value semantics. + +```csharp +// Single-value object +public readonly record struct OrderId(string Value) +{ + public OrderId(string value) : this( + !string.IsNullOrWhiteSpace(value) + ? value + : throw new ArgumentException("OrderId cannot be empty", nameof(value))) + { + } + + public override string ToString() => Value; + + // NO implicit conversions - defeats type safety! + // Access inner value explicitly: orderId.Value +} + +// Multi-value object +public readonly record struct Money(decimal Amount, string Currency) +{ + public Money(decimal amount, string currency) : this( + amount >= 0 ? amount : throw new ArgumentException("Amount cannot be negative", nameof(amount)), + ValidateCurrency(currency)) + { + } + + private static string ValidateCurrency(string currency) + { + if (string.IsNullOrWhiteSpace(currency) || currency.Length != 3) + throw new ArgumentException("Currency must be a 3-letter code", nameof(currency)); + return currency.ToUpperInvariant(); + } + + public Money Add(Money other) + { + if (Currency != other.Currency) + throw new InvalidOperationException($"Cannot add {Currency} to {other.Currency}"); + + return new Money(Amount + other.Amount, Currency); + } + + public override string ToString() => $"{Amount:N2} {Currency}"; +} + +// Complex value object with factory pattern +public readonly record struct PhoneNumber +{ + public string Value { get; } + + private PhoneNumber(string value) => Value = value; + + public static Result Create(string input) + { + if (string.IsNullOrWhiteSpace(input)) + return Result.Failure("Phone number cannot be empty"); + + // Normalize: remove all non-digits + var digits = new string(input.Where(char.IsDigit).ToArray()); + + if (digits.Length < 10 || digits.Length > 15) + return Result.Failure("Phone number must be 10-15 digits"); + + return Result.Success(new PhoneNumber(digits)); + } + + public override string ToString() => Value; +} + +// Percentage value object with range validation +public readonly record struct Percentage +{ + private readonly decimal _value; + + public decimal Value => _value; + + public Percentage(decimal value) + { + if (value < 0 || value > 100) + throw new ArgumentOutOfRangeException(nameof(value), "Percentage must be between 0 and 100"); + _value = value; + } + + public decimal AsDecimal() => _value / 100m; + + public static Percentage FromDecimal(decimal decimalValue) + { + if (decimalValue < 0 || decimalValue > 1) + throw new ArgumentOutOfRangeException(nameof(decimalValue), "Decimal must be between 0 and 1"); + return new Percentage(decimalValue * 100); + } + + public override string ToString() => $"{_value}%"; +} + +// Strongly-typed ID +public readonly record struct CustomerId(Guid Value) +{ + public static CustomerId New() => new(Guid.NewGuid()); + public override string ToString() => Value.ToString(); +} + +// Quantity with units +public readonly record struct Quantity(int Value, string Unit) +{ + public Quantity(int value, string unit) : this( + value >= 0 ? value : throw new ArgumentException("Quantity cannot be negative"), + !string.IsNullOrWhiteSpace(unit) ? unit : throw new ArgumentException("Unit cannot be empty")) + { + } + + public override string ToString() => $"{Value} {Unit}"; +} +``` + +**Why `readonly record struct` for value objects:** +- **Value semantics**: Equality based on content, not reference +- **Stack allocation**: Better performance, no GC pressure +- **Immutability**: `readonly` prevents accidental mutation +- **Pattern matching**: Works seamlessly with switch expressions + +## No Implicit Conversions + +**CRITICAL: NO implicit conversions.** Implicit operators defeat the purpose of value objects by allowing silent type coercion: + +```csharp +// WRONG - defeats compile-time safety: +public readonly record struct UserId(Guid Value) +{ + public static implicit operator UserId(Guid value) => new(value); // NO! + public static implicit operator Guid(UserId value) => value.Value; // NO! +} + +// With implicit operators, this compiles silently: +void ProcessUser(UserId userId) { } +ProcessUser(Guid.NewGuid()); // Oops - meant to pass PostId + +// CORRECT - all conversions explicit: +public readonly record struct UserId(Guid Value) +{ + public static UserId New() => new(Guid.NewGuid()); + // No implicit operators + // Create: new UserId(guid) or UserId.New() + // Extract: userId.Value +} +``` + +Explicit conversions force every boundary crossing to be visible: + +```csharp +// API boundary - explicit conversion IN +var userId = new UserId(request.UserId); // Validates on entry + +// Database boundary - explicit conversion OUT +await _db.ExecuteAsync(sql, new { UserId = userId.Value }); +``` + +## Pattern Matching (C# 8-12) + +Leverage modern pattern matching for cleaner, more expressive code. + +```csharp +// Switch expressions with value objects +public string GetPaymentMethodDescription(PaymentMethod payment) => payment switch +{ + { Type: PaymentType.CreditCard, Last4: var last4 } => $"Credit card ending in {last4}", + { Type: PaymentType.BankTransfer, AccountNumber: var account } => $"Bank transfer from {account}", + { Type: PaymentType.Cash } => "Cash payment", + _ => "Unknown payment method" +}; + +// Property patterns +public decimal CalculateDiscount(Order order) => order switch +{ + { Total: > 1000m } => order.Total * 0.15m, + { Total: > 500m } => order.Total * 0.10m, + { Total: > 100m } => order.Total * 0.05m, + _ => 0m +}; + +// Relational and logical patterns +public string ClassifyTemperature(int temp) => temp switch +{ + < 0 => "Freezing", + >= 0 and < 10 => "Cold", + >= 10 and < 20 => "Cool", + >= 20 and < 30 => "Warm", + >= 30 => "Hot", + _ => throw new ArgumentOutOfRangeException(nameof(temp)) +}; + +// List patterns (C# 11+) +public bool IsValidSequence(int[] numbers) => numbers switch +{ + [] => false, // Empty + [_] => true, // Single element + [var first, .., var last] when first < last => true, // First < last + _ => false +}; + +// Type patterns with null checks +public string FormatValue(object? value) => value switch +{ + null => "null", + string s => $"\"{s}\"", + int i => i.ToString(), + double d => d.ToString("F2"), + DateTime dt => dt.ToString("yyyy-MM-dd"), + Money m => m.ToString(), + IEnumerable collection => $"[{string.Join(", ", collection)}]", + _ => value.ToString() ?? "unknown" +}; + +// Combining patterns for complex logic +public record OrderState(bool IsPaid, bool IsShipped, bool IsCancelled); + +public string GetOrderStatus(OrderState state) => state switch +{ + { IsCancelled: true } => "Cancelled", + { IsPaid: true, IsShipped: true } => "Delivered", + { IsPaid: true, IsShipped: false } => "Processing", + { IsPaid: false } => "Awaiting Payment", + _ => "Unknown" +}; + +// Pattern matching with value objects +public decimal CalculateShipping(Money total, Country destination) => (total, destination) switch +{ + ({ Amount: > 100m }, _) => 0m, // Free shipping over $100 + (_, { Code: "US" or "CA" }) => 5m, // North America + (_, { Code: "GB" or "FR" or "DE" }) => 10m, // Europe + _ => 25m // International +}; +``` diff --git a/.github/skills/csharp-type-design-performance/SKILL.md b/.github/skills/csharp-type-design-performance/SKILL.md new file mode 100644 index 00000000..1d693bf5 --- /dev/null +++ b/.github/skills/csharp-type-design-performance/SKILL.md @@ -0,0 +1,398 @@ +--- +name: type-design-performance +description: Design .NET types for performance. Seal classes, use readonly structs, prefer static pure functions, avoid premature enumeration, and choose the right collection types. +invocable: false +--- + +# Type Design for Performance + +## When to Use This Skill + +Use this skill when: +- Designing new types and APIs +- Reviewing code for performance issues +- Choosing between class, struct, and record +- Working with collections and enumerables + +--- + +## Core Principles + +1. **Seal your types** - Unless explicitly designed for inheritance +2. **Prefer readonly structs** - For small, immutable value types +3. **Prefer static pure functions** - Better performance and testability +4. **Defer enumeration** - Don't materialize until you need to +5. **Return immutable collections** - From API boundaries + +--- + +## Seal Classes by Default + +Sealing classes enables JIT devirtualization and communicates API intent. + +```csharp +// DO: Seal classes not designed for inheritance +public sealed class OrderProcessor +{ + public void Process(Order order) { } +} + +// DO: Seal records (they're classes) +public sealed record OrderCreated(OrderId Id, CustomerId CustomerId); + +// DON'T: Leave unsealed without reason +public class OrderProcessor // Can be subclassed - intentional? +{ + public virtual void Process(Order order) { } // Virtual = slower +} +``` + +**Benefits:** +- JIT can devirtualize method calls +- Communicates "this is not an extension point" +- Prevents accidental breaking changes + +--- + +## Readonly Structs for Value Types + +Structs should be `readonly` when immutable. This prevents defensive copies. + +```csharp +// DO: Readonly struct for immutable value types +public readonly record struct OrderId(Guid Value) +{ + public static OrderId New() => new(Guid.NewGuid()); + public override string ToString() => Value.ToString(); +} + +// DO: Readonly struct for small, short-lived data +public readonly struct Money +{ + public decimal Amount { get; } + public string Currency { get; } + + public Money(decimal amount, string currency) + { + Amount = amount; + Currency = currency; + } +} + +// DON'T: Mutable struct (causes defensive copies) +public struct Point // Not readonly! +{ + public int X { get; set; } // Mutable! + public int Y { get; set; } +} +``` + +### When to Use Structs + +| Use Struct When | Use Class When | +|-----------------|----------------| +| Small (≤16 bytes typically) | Larger objects | +| Short-lived | Long-lived | +| Frequently allocated | Shared references needed | +| Value semantics required | Identity semantics required | +| Immutable | Mutable state | + +--- + +## Prefer Static Pure Functions + +Static methods with no side effects are faster and more testable. + +```csharp +// DO: Static pure function +public static class OrderCalculator +{ + public static Money CalculateTotal(IReadOnlyList items) + { + var total = items.Sum(i => i.Price * i.Quantity); + return new Money(total, "USD"); + } +} + +// Usage - predictable, testable +var total = OrderCalculator.CalculateTotal(items); +``` + +**Benefits:** +- No vtable lookup (faster) +- No hidden state +- Easier to test (pure input → output) +- Thread-safe by design +- Forces explicit dependencies + +```csharp +// DON'T: Instance method hiding dependencies +public class OrderCalculator +{ + private readonly ITaxService _taxService; // Hidden dependency + private readonly IDiscountService _discountService; // Hidden dependency + + public Money CalculateTotal(IReadOnlyList items) + { + // What does this actually depend on? + } +} + +// BETTER: Explicit dependencies via parameters +public static class OrderCalculator +{ + public static Money CalculateTotal( + IReadOnlyList items, + decimal taxRate, + decimal discountPercent) + { + // All inputs visible + } +} +``` + +**Don't go overboard** - Use instance methods when you genuinely need state or polymorphism. + +--- + +## Defer Enumeration + +Don't materialize enumerables until necessary. Avoid excessive LINQ chains. + +```csharp +// BAD: Premature materialization +public IReadOnlyList GetActiveOrders() +{ + return _orders + .Where(o => o.IsActive) + .ToList() // Materialized! + .OrderBy(o => o.CreatedAt) // Another iteration + .ToList(); // Materialized again! +} + +// GOOD: Defer until the end +public IReadOnlyList GetActiveOrders() +{ + return _orders + .Where(o => o.IsActive) + .OrderBy(o => o.CreatedAt) + .ToList(); // Single materialization +} + +// GOOD: Return IEnumerable if caller might not need all items +public IEnumerable GetActiveOrders() +{ + return _orders + .Where(o => o.IsActive) + .OrderBy(o => o.CreatedAt); + // Caller decides when to materialize +} +``` + +### Async Enumeration + +Be careful with async and IEnumerable: + +```csharp +// BAD: Async in LINQ - hidden allocations +var results = orders + .Select(async o => await ProcessOrderAsync(o)) // Task per item! + .ToList(); +await Task.WhenAll(results); + +// GOOD: Use IAsyncEnumerable for streaming +public async IAsyncEnumerable ProcessOrdersAsync( + IEnumerable orders, + [EnumeratorCancellation] CancellationToken ct = default) +{ + foreach (var order in orders) + { + ct.ThrowIfCancellationRequested(); + yield return await ProcessOrderAsync(order, ct); + } +} + +// GOOD: Batch processing for parallelism +var results = await Task.WhenAll( + orders.Select(o => ProcessOrderAsync(o))); +``` + +--- + +## ValueTask vs Task + +Use `ValueTask` for hot paths that often complete synchronously. For real I/O, just use `Task`. + +```csharp +// DO: ValueTask for cached/synchronous paths +public ValueTask GetUserAsync(UserId id) +{ + if (_cache.TryGetValue(id, out var user)) + { + return ValueTask.FromResult(user); // No allocation + } + + return new ValueTask(FetchUserAsync(id)); +} + +// DO: Task for real I/O (simpler, no footguns) +public Task CreateOrderAsync(CreateOrderCommand cmd) +{ + // This always hits the database + return _repository.CreateAsync(cmd); +} +``` + +**ValueTask rules:** +- Never await a ValueTask more than once +- Never use `.Result` or `.GetAwaiter().GetResult()` before completion +- If in doubt, use Task + +--- + +## Span and Memory for Bytes + +Use `Span` and `Memory` instead of `byte[]` for low-level operations. + +```csharp +// DO: Accept Span for synchronous operations +public static int ParseInt(ReadOnlySpan text) +{ + return int.Parse(text); +} + +// DO: Accept Memory for async operations +public async Task WriteAsync(ReadOnlyMemory data) +{ + await _stream.WriteAsync(data); +} + +// DON'T: Force array allocation +public static int ParseInt(string text) // String allocated +{ + return int.Parse(text); +} +``` + +### Common Span Patterns + +```csharp +// Slice without allocation +ReadOnlySpan span = "Hello, World!".AsSpan(); +var hello = span[..5]; // No allocation + +// Stack allocation for small buffers +Span buffer = stackalloc byte[256]; + +// Use ArrayPool for larger buffers +var buffer = ArrayPool.Shared.Rent(4096); +try +{ + // Use buffer... +} +finally +{ + ArrayPool.Shared.Return(buffer); +} +``` + +--- + +## Collection Return Types + +### Return Immutable Collections from APIs + +```csharp +// DO: Return immutable collection +public IReadOnlyList GetOrders() +{ + return _orders.ToList(); // Caller can't modify internal state +} + +// DO: Use frozen collections for static data (.NET 8+) +private static readonly FrozenDictionary _handlers = + new Dictionary + { + ["create"] = new CreateHandler(), + ["update"] = new UpdateHandler(), + }.ToFrozenDictionary(); + +// DON'T: Return mutable collection +public List GetOrders() +{ + return _orders; // Caller can modify! +} +``` + +### Internal Mutation is Fine + +```csharp +public IReadOnlyList BuildOrderItems(Cart cart) +{ + var items = new List(); // Mutable internally + + foreach (var cartItem in cart.Items) + { + items.Add(CreateOrderItem(cartItem)); + } + + return items; // Return as IReadOnlyList +} +``` + +### Collection Guidelines + +| Scenario | Return Type | +|----------|-------------| +| API boundary | `IReadOnlyList`, `IReadOnlyCollection` | +| Static lookup data | `FrozenDictionary`, `FrozenSet` | +| Internal building | `List`, then return as readonly | +| Single item or none | `T?` (nullable) | +| Zero or more, lazy | `IEnumerable` | + +--- + +## Quick Reference + +| Pattern | Benefit | +|---------|---------| +| `sealed class` | Devirtualization, clear API | +| `readonly record struct` | No defensive copies, value semantics | +| Static pure functions | No vtable, testable, thread-safe | +| Defer `.ToList()` | Single materialization | +| `ValueTask` for hot paths | Avoid Task allocation | +| `Span` for bytes | Stack allocation, no copying | +| `IReadOnlyList` return | Immutable API contract | +| `FrozenDictionary` | Fastest lookup for static data | + +--- + +## Anti-Patterns + +```csharp +// DON'T: Unsealed class without reason +public class OrderService { } // Seal it! + +// DON'T: Mutable struct +public struct Point { public int X; public int Y; } // Make readonly + +// DON'T: Instance method that could be static +public int Add(int a, int b) => a + b; // Make static + +// DON'T: Multiple ToList() calls +items.Where(...).ToList().OrderBy(...).ToList(); // One ToList at end + +// DON'T: Return List from public API +public List GetOrders(); // Return IReadOnlyList + +// DON'T: ValueTask for always-async operations +public ValueTask CreateOrderAsync(); // Just use Task +``` + +--- + +## Resources + +- **Performance Best Practices**: https://learn.microsoft.com/en-us/dotnet/standard/performance/ +- **Span Guidance**: https://learn.microsoft.com/en-us/dotnet/standard/memory-and-spans/ +- **Frozen Collections**: https://learn.microsoft.com/en-us/dotnet/api/system.collections.frozen diff --git a/.github/skills/ilspy-decompile/SKILL.md b/.github/skills/ilspy-decompile/SKILL.md new file mode 100644 index 00000000..bfc36df1 --- /dev/null +++ b/.github/skills/ilspy-decompile/SKILL.md @@ -0,0 +1,102 @@ +--- +name: ilspy-decompile +description: Understand implementation details of .NET code by decompiling assemblies. Use when you want to see how a .NET API works internally, inspect NuGet package source, view framework implementation, or understand compiled .NET binaries. +allowed-tools: Bash(dnx:*) +--- + +# .NET Assembly Decompilation with ILSpy + +Use this skill to understand how .NET code works internally by decompiling compiled assemblies. + +## Prerequisites + +- .NET SDK installed +- ILSpy command-line tool available via one of the following: + - `dnx ilspycmd` (if available in your SDK or runtime) + - `dotnet tool install --global ilspycmd` + +Both forms are shown below. Use the one that works in your environment. + +> Note: ILSpyCmd options may vary slightly by version. +> Always verify supported flags with `ilspycmd -h`. + +## Quick start + +```bash +# Decompile an assembly to stdout +ilspycmd MyLibrary.dll +# or +dnx ilspycmd MyLibrary.dll + +# Decompile to an output folder +ilspycmd -o output-folder MyLibrary.dll +``` + +## Common .NET Assembly Locations + +### NuGet packages + +```bash +~/.nuget/packages///lib// +``` + +### .NET runtime libraries + +```bash +dotnet --list-runtimes +``` + +### .NET SDK reference assemblies + +```bash +dotnet --list-sdks +``` + +> Reference assemblies do not contain implementations. + +### Project build output + +```bash +./bin/Debug/net8.0/.dll +./bin/Release/net8.0/publish/.dll +``` + +## Core workflow + +1. Identify what you want to understand +2. Locate the assembly +3. List types +4. Decompile the target + +## Commands + +### Basic decompilation + +```bash +ilspycmd MyLibrary.dll +ilspycmd -o ./decompiled MyLibrary.dll +ilspycmd -p -o ./project MyLibrary.dll +``` + +### Targeted decompilation + +```bash +ilspycmd -t Namespace.ClassName MyLibrary.dll +ilspycmd -lv CSharp12_0 MyLibrary.dll +``` + +### View IL code + +```bash +ilspycmd -il MyLibrary.dll +``` + +## Notes on modern .NET builds + +- ReadyToRun images may reduce readability +- Trimmed or AOT builds may omit code +- Prefer non-trimmed builds + +## Legal note + +Decompiling assemblies may be subject to license restrictions. diff --git a/.github/skills/microsoft-extensions-configuration/SKILL.md b/.github/skills/microsoft-extensions-configuration/SKILL.md new file mode 100644 index 00000000..3534f77d --- /dev/null +++ b/.github/skills/microsoft-extensions-configuration/SKILL.md @@ -0,0 +1,343 @@ +--- +name: microsoft-extensions-configuration +description: Microsoft.Extensions.Options patterns including IValidateOptions, strongly-typed settings, validation on startup, and the Options pattern for clean configuration management. +invocable: false +--- + +# Microsoft.Extensions Configuration Patterns + +## When to Use This Skill + +Use this skill when: +- Binding configuration from appsettings.json to strongly-typed classes +- Validating configuration at application startup (fail fast) +- Implementing complex validation logic for settings +- Designing configuration classes that are testable and maintainable +- Understanding IOptions, IOptionsSnapshot, and IOptionsMonitor + +## Reference Files + +- [advanced-patterns.md](advanced-patterns.md): Validators with dependencies, named options, complete production example (AkkaSettings), and testing validators + +## Why Configuration Validation Matters + +**The Problem:** Applications often fail at runtime due to misconfiguration - missing connection strings, invalid URLs, out-of-range values. These failures happen deep in business logic, far from where configuration is loaded. + +**The Solution:** Validate configuration at startup. If invalid, fail immediately with a clear error message. + +```csharp +// BAD: Fails at runtime when someone tries to use the service +public class EmailService +{ + public EmailService(IOptions options) + { + var settings = options.Value; + // Throws NullReferenceException 10 minutes into production + _client = new SmtpClient(settings.Host, settings.Port); + } +} + +// GOOD: Fails at startup with clear error +// "SmtpSettings validation failed: Host is required" +``` + +--- + +## Pattern 1: Basic Options Binding + +### Define a Settings Class + +```csharp +public class SmtpSettings +{ + public const string SectionName = "Smtp"; + + public string Host { get; set; } = string.Empty; + public int Port { get; set; } = 587; + public string? Username { get; set; } + public string? Password { get; set; } + public bool UseSsl { get; set; } = true; +} +``` + +### Bind from Configuration + +```csharp +builder.Services.AddOptions() + .BindConfiguration(SmtpSettings.SectionName); + +// appsettings.json +{ + "Smtp": { + "Host": "smtp.example.com", + "Port": 587, + "Username": "user@example.com", + "Password": "secret", + "UseSsl": true + } +} +``` + +### Consume in Services + +```csharp +public class EmailService +{ + private readonly SmtpSettings _settings; + + // IOptions - singleton, read once at startup + public EmailService(IOptions options) + { + _settings = options.Value; + } +} +``` + +--- + +## Pattern 2: Data Annotations Validation + +For simple validation rules, use Data Annotations: + +```csharp +using System.ComponentModel.DataAnnotations; + +public class SmtpSettings +{ + public const string SectionName = "Smtp"; + + [Required(ErrorMessage = "SMTP host is required")] + public string Host { get; set; } = string.Empty; + + [Range(1, 65535, ErrorMessage = "Port must be between 1 and 65535")] + public int Port { get; set; } = 587; + + [EmailAddress(ErrorMessage = "Username must be a valid email address")] + public string? Username { get; set; } + + public string? Password { get; set; } + public bool UseSsl { get; set; } = true; +} +``` + +### Enable Data Annotations Validation + +```csharp +builder.Services.AddOptions() + .BindConfiguration(SmtpSettings.SectionName) + .ValidateDataAnnotations() // Enable attribute-based validation + .ValidateOnStart(); // Validate immediately at startup +``` + +**Key Point:** `.ValidateOnStart()` is critical. Without it, validation only runs when the options are first accessed. + +--- + +## Pattern 3: IValidateOptions for Complex Validation + +Data Annotations work for simple rules, but complex validation requires `IValidateOptions`: + +| Scenario | Data Annotations | IValidateOptions | +|----------|------------------|------------------| +| Required field | Yes | Yes | +| Range check | Yes | Yes | +| Cross-property validation | No | Yes | +| Conditional validation | No | Yes | +| External service checks | No | Yes | +| Dependency injection in validator | No | Yes | + +### Implementing IValidateOptions + +```csharp +using Microsoft.Extensions.Options; + +public class SmtpSettingsValidator : IValidateOptions +{ + public ValidateOptionsResult Validate(string? name, SmtpSettings options) + { + var failures = new List(); + + if (string.IsNullOrWhiteSpace(options.Host)) + failures.Add("Host is required"); + + if (options.Port is < 1 or > 65535) + failures.Add($"Port {options.Port} is invalid. Must be between 1 and 65535"); + + // Cross-property validation + if (!string.IsNullOrEmpty(options.Username) && string.IsNullOrEmpty(options.Password)) + failures.Add("Password is required when Username is specified"); + + // Conditional validation + if (options.UseSsl && options.Port == 25) + failures.Add("Port 25 is typically not used with SSL. Consider port 465 or 587"); + + return failures.Count > 0 + ? ValidateOptionsResult.Fail(failures) + : ValidateOptionsResult.Success; + } +} +``` + +### Register the Validator + +```csharp +builder.Services.AddOptions() + .BindConfiguration(SmtpSettings.SectionName) + .ValidateDataAnnotations() + .ValidateOnStart(); + +builder.Services.AddSingleton, SmtpSettingsValidator>(); +``` + +**Order matters:** Data Annotations run first, then IValidateOptions validators. All failures are collected together. + +See [advanced-patterns.md](advanced-patterns.md) for validators with dependencies, named options, and a complete production example. + +--- + +## Pattern 4: Options Lifetime + +| Interface | Lifetime | Reloads on Change | Use Case | +|-----------|----------|-------------------|----------| +| `IOptions` | Singleton | No | Static config, read once | +| `IOptionsSnapshot` | Scoped | Yes (per request) | Web apps needing fresh config | +| `IOptionsMonitor` | Singleton | Yes (with callback) | Background services, real-time updates | + +### IOptionsMonitor for Background Services + +```csharp +public class BackgroundWorker : BackgroundService +{ + private readonly IOptionsMonitor _optionsMonitor; + private WorkerSettings _currentSettings; + + public BackgroundWorker(IOptionsMonitor optionsMonitor) + { + _optionsMonitor = optionsMonitor; + _currentSettings = optionsMonitor.CurrentValue; + + _optionsMonitor.OnChange(settings => + { + _currentSettings = settings; + }); + } + + protected override async Task ExecuteAsync(CancellationToken stoppingToken) + { + while (!stoppingToken.IsCancellationRequested) + { + await DoWorkAsync(); + await Task.Delay(_currentSettings.PollingInterval, stoppingToken); + } + } +} +``` + +--- + +## Pattern 5: Post-Configuration + +Modify options after binding but before validation: + +```csharp +builder.Services.AddOptions() + .BindConfiguration("Api") + .PostConfigure(options => + { + if (!string.IsNullOrEmpty(options.BaseUrl) && !options.BaseUrl.EndsWith('/')) + options.BaseUrl += '/'; + + options.Timeout ??= TimeSpan.FromSeconds(30); + }) + .ValidateDataAnnotations() + .ValidateOnStart(); +``` + +--- + +## Anti-Patterns to Avoid + +### 1. Manual Configuration Access + +```csharp +// BAD: Bypasses validation, hard to test +public class MyService +{ + public MyService(IConfiguration configuration) + { + var host = configuration["Smtp:Host"]; // No validation! + } +} + +// GOOD: Strongly-typed, validated +public class MyService +{ + public MyService(IOptions options) + { + var host = options.Value.Host; // Validated at startup + } +} +``` + +### 2. Validation in Constructor + +```csharp +// BAD: Validation happens at runtime, not startup +public class MyService +{ + public MyService(IOptions options) + { + if (string.IsNullOrEmpty(options.Value.Required)) + throw new ArgumentException("Required is missing"); // Too late! + } +} + +// GOOD: Validation at startup via IValidateOptions + ValidateOnStart() +``` + +### 3. Forgetting ValidateOnStart + +```csharp +// BAD: Validation only runs when first accessed +builder.Services.AddOptions() + .ValidateDataAnnotations(); // Missing ValidateOnStart! + +// GOOD: Fails immediately if invalid +builder.Services.AddOptions() + .ValidateDataAnnotations() + .ValidateOnStart(); +``` + +### 4. Throwing in IValidateOptions + +```csharp +// BAD: Throws exception, breaks validation chain +public ValidateOptionsResult Validate(string? name, Settings options) +{ + if (options.Value < 0) + throw new ArgumentException("Value cannot be negative"); // Wrong! + return ValidateOptionsResult.Success; +} + +// GOOD: Return failure result +public ValidateOptionsResult Validate(string? name, Settings options) +{ + if (options.Value < 0) + return ValidateOptionsResult.Fail("Value cannot be negative"); + return ValidateOptionsResult.Success; +} +``` + +--- + +## Summary + +| Principle | Implementation | +|-----------|----------------| +| Fail fast | `.ValidateOnStart()` | +| Strongly-typed | Bind to POCO classes | +| Simple validation | Data Annotations | +| Complex validation | `IValidateOptions` | +| Cross-property rules | `IValidateOptions` | +| Environment-aware | Inject `IHostEnvironment` | +| Testable | Validators are plain classes | diff --git a/.github/skills/microsoft-extensions-configuration/advanced-patterns.md b/.github/skills/microsoft-extensions-configuration/advanced-patterns.md new file mode 100644 index 00000000..e9037e05 --- /dev/null +++ b/.github/skills/microsoft-extensions-configuration/advanced-patterns.md @@ -0,0 +1,325 @@ +# Advanced Configuration Patterns + +Validators with dependencies, named options, complete production example, and testing configuration validators. + +## Contents + +- [Validators with Dependencies](#validators-with-dependencies) +- [Named Options](#named-options) +- [Complete Example - Production Settings Class](#complete-example---production-settings-class) +- [Testing Configuration Validators](#testing-configuration-validators) + +## Validators with Dependencies + +IValidateOptions validators are resolved from DI, so they can have dependencies: + +```csharp +public class DatabaseSettingsValidator : IValidateOptions +{ + private readonly ILogger _logger; + private readonly IHostEnvironment _environment; + + public DatabaseSettingsValidator( + ILogger logger, + IHostEnvironment environment) + { + _logger = logger; + _environment = environment; + } + + public ValidateOptionsResult Validate(string? name, DatabaseSettings options) + { + var failures = new List(); + + if (string.IsNullOrWhiteSpace(options.ConnectionString)) + { + failures.Add("ConnectionString is required"); + } + + // Environment-specific validation + if (_environment.IsProduction()) + { + if (options.ConnectionString?.Contains("localhost") == true) + { + failures.Add("Production cannot use localhost database"); + } + + if (!options.ConnectionString?.Contains("Encrypt=True") == true) + { + _logger.LogWarning("Production database connection should use encryption"); + } + } + + // Validate connection string format + if (!string.IsNullOrEmpty(options.ConnectionString)) + { + try + { + var builder = new SqlConnectionStringBuilder(options.ConnectionString); + if (string.IsNullOrEmpty(builder.DataSource)) + { + failures.Add("ConnectionString must specify a Data Source"); + } + } + catch (Exception ex) + { + failures.Add($"ConnectionString is malformed: {ex.Message}"); + } + } + + return failures.Count > 0 + ? ValidateOptionsResult.Fail(failures) + : ValidateOptionsResult.Success; + } +} +``` + +## Named Options + +When you have multiple instances of the same settings type (e.g., multiple database connections): + +```csharp +// appsettings.json +{ + "Databases": { + "Primary": { + "ConnectionString": "Server=primary;..." + }, + "Replica": { + "ConnectionString": "Server=replica;..." + } + } +} + +// Registration +builder.Services.AddOptions("Primary") + .BindConfiguration("Databases:Primary") + .ValidateDataAnnotations() + .ValidateOnStart(); + +builder.Services.AddOptions("Replica") + .BindConfiguration("Databases:Replica") + .ValidateDataAnnotations() + .ValidateOnStart(); + +// Consumption +public class DataService +{ + private readonly DatabaseSettings _primary; + private readonly DatabaseSettings _replica; + + public DataService(IOptionsSnapshot options) + { + _primary = options.Get("Primary"); + _replica = options.Get("Replica"); + } +} +``` + +### Named Options Validator + +```csharp +public class DatabaseSettingsValidator : IValidateOptions +{ + public ValidateOptionsResult Validate(string? name, DatabaseSettings options) + { + var failures = new List(); + var prefix = string.IsNullOrEmpty(name) ? "" : $"[{name}] "; + + if (string.IsNullOrWhiteSpace(options.ConnectionString)) + { + failures.Add($"{prefix}ConnectionString is required"); + } + + // Name-specific validation + if (name == "Primary" && options.ReadOnly) + { + failures.Add("Primary database cannot be read-only"); + } + + return failures.Count > 0 + ? ValidateOptionsResult.Fail(failures) + : ValidateOptionsResult.Success; + } +} +``` + +## Complete Example - Production Settings Class + +```csharp +using System.ComponentModel.DataAnnotations; +using Microsoft.Extensions.Options; + +public class AkkaSettings +{ + public const string SectionName = "AkkaSettings"; + + [Required] + public string ActorSystemName { get; set; } = "MySystem"; + + public AkkaExecutionMode ExecutionMode { get; set; } = AkkaExecutionMode.LocalTest; + + public bool LogConfigOnStart { get; set; } = false; + + public RemoteOptions RemoteOptions { get; set; } = new(); + + public ClusterOptions ClusterOptions { get; set; } = new(); + + public ClusterBootstrapOptions ClusterBootstrapOptions { get; set; } = new(); +} + +public enum AkkaExecutionMode +{ + LocalTest, // No remoting, no clustering + Clustered // Full cluster with sharding, distributed pub/sub +} + +public class AkkaSettingsValidator : IValidateOptions +{ + private readonly IHostEnvironment _environment; + + public AkkaSettingsValidator(IHostEnvironment environment) + { + _environment = environment; + } + + public ValidateOptionsResult Validate(string? name, AkkaSettings options) + { + var failures = new List(); + + // Basic validation + if (string.IsNullOrWhiteSpace(options.ActorSystemName)) + { + failures.Add("ActorSystemName is required"); + } + + // Mode-specific validation + if (options.ExecutionMode == AkkaExecutionMode.Clustered) + { + ValidateClusteredMode(options, failures); + } + + // Environment-specific validation + if (_environment.IsProduction() && options.ExecutionMode == AkkaExecutionMode.LocalTest) + { + failures.Add("LocalTest execution mode is not allowed in production"); + } + + return failures.Count > 0 + ? ValidateOptionsResult.Fail(failures) + : ValidateOptionsResult.Success; + } + + private void ValidateClusteredMode(AkkaSettings options, List failures) + { + if (string.IsNullOrEmpty(options.RemoteOptions.PublicHostName)) + { + failures.Add("RemoteOptions.PublicHostName is required in Clustered mode"); + } + + if (options.RemoteOptions.Port is null or < 0) + { + failures.Add("RemoteOptions.Port must be >= 0 in Clustered mode"); + } + + if (options.ClusterBootstrapOptions.Enabled) + { + ValidateClusterBootstrap(options.ClusterBootstrapOptions, failures); + } + else if (options.ClusterOptions.SeedNodes?.Length == 0) + { + failures.Add("Either ClusterBootstrap must be enabled or SeedNodes must be specified"); + } + } + + private void ValidateClusterBootstrap(ClusterBootstrapOptions options, List failures) + { + if (string.IsNullOrEmpty(options.ServiceName)) + { + failures.Add("ClusterBootstrapOptions.ServiceName is required"); + } + + if (options.RequiredContactPointsNr <= 0) + { + failures.Add("ClusterBootstrapOptions.RequiredContactPointsNr must be > 0"); + } + + switch (options.DiscoveryMethod) + { + case DiscoveryMethod.Config: + if (options.ConfigServiceEndpoints?.Length == 0) + { + failures.Add("ConfigServiceEndpoints required for Config discovery"); + } + break; + + case DiscoveryMethod.AzureTableStorage: + if (options.AzureDiscoveryOptions == null) + { + failures.Add("AzureDiscoveryOptions required for Azure discovery"); + } + break; + } + } +} + +// Registration +builder.Services.AddOptions() + .BindConfiguration(AkkaSettings.SectionName) + .ValidateDataAnnotations() + .ValidateOnStart(); + +builder.Services.AddSingleton, AkkaSettingsValidator>(); +``` + +## Testing Configuration Validators + +```csharp +public class SmtpSettingsValidatorTests +{ + private readonly SmtpSettingsValidator _validator = new(); + + [Fact] + public void Validate_WithValidSettings_ReturnsSuccess() + { + var settings = new SmtpSettings + { + Host = "smtp.example.com", + Port = 587, + Username = "user@example.com", + Password = "secret" + }; + + var result = _validator.Validate(null, settings); + + result.Succeeded.Should().BeTrue(); + } + + [Fact] + public void Validate_WithMissingHost_ReturnsFail() + { + var settings = new SmtpSettings { Host = "" }; + + var result = _validator.Validate(null, settings); + + result.Succeeded.Should().BeFalse(); + result.FailureMessage.Should().Contain("Host is required"); + } + + [Fact] + public void Validate_WithUsernameButNoPassword_ReturnsFail() + { + var settings = new SmtpSettings + { + Host = "smtp.example.com", + Username = "user@example.com", + Password = null // Missing! + }; + + var result = _validator.Validate(null, settings); + + result.Succeeded.Should().BeFalse(); + result.FailureMessage.Should().Contain("Password is required"); + } +} +``` diff --git a/.github/skills/package-management/SKILL.md b/.github/skills/package-management/SKILL.md new file mode 100644 index 00000000..eafad0bb --- /dev/null +++ b/.github/skills/package-management/SKILL.md @@ -0,0 +1,459 @@ +--- +name: package-management +description: Manage NuGet packages using Central Package Management (CPM) and dotnet CLI commands. Never edit XML directly - use dotnet add/remove/list commands. Use shared version variables for related packages. +invocable: false +--- + +# NuGet Package Management + +## When to Use This Skill + +Use this skill when: +- Adding, removing, or updating NuGet packages +- Setting up Central Package Management (CPM) for a solution +- Managing package versions across multiple projects +- Troubleshooting package conflicts or restore issues + +--- + +## Golden Rule: Never Edit XML Directly + +**Always use `dotnet` CLI commands to manage packages.** Never manually edit `.csproj` or `Directory.Packages.props` files. + +```bash +# DO: Use CLI commands +dotnet add package Newtonsoft.Json +dotnet remove package Newtonsoft.Json +dotnet list package --outdated + +# DON'T: Edit XML directly +# +``` + +**Why:** +- CLI validates package exists and resolves correct version +- Handles transitive dependencies correctly +- Updates lock files if present +- Avoids typos and malformed XML +- Works correctly with CPM + +--- + +## Central Package Management (CPM) + +CPM centralizes all package versions in one file, eliminating version conflicts across projects. + +### Enable CPM + +Create `Directory.Packages.props` in solution root: + +```xml + + + true + + + + + + + + +``` + +### Project Files with CPM + +Projects reference packages **without versions**: + +```xml + + + + + + + +``` + +### Adding Packages with CPM + +```bash +# Adds to Directory.Packages.props AND project file +dotnet add package Serilog.Sinks.Console + +# Result in Directory.Packages.props: +# + +# Result in project file: +# +``` + +--- + +## Shared Version Variables + +Group related packages with shared version variables: + +```xml + + + true + + + + + 1.5.59 + 1.5.59 + 9.0.0 + 1.11.0 + 2.9.2 + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +``` + +**Benefits:** +- Update all Akka packages by changing one variable +- Clear organization with labeled ItemGroups +- Prevents version mismatches in related packages + +--- + +## When NOT to Use CPM + +Central Package Management isn't always the right choice: + +### Legacy Projects + +Migrating an existing large solution to CPM can introduce issues: +- Existing version conflicts become visible all at once +- Some packages may have intentional version differences +- Migration requires touching many files simultaneously + +**Recommendation**: For legacy projects, migrate incrementally or stick with per-project versioning if it's working. + +### Version Ranges + +CPM requires exact versions - it doesn't support version ranges: + +```xml + + + + + +``` + +If you need version ranges (rare, but some library scenarios require it), CPM won't work. + +### Older .NET Versions + +CPM requires: +- **.NET SDK 6.0.300+** or later +- **NuGet 6.2+** or later +- **Visual Studio 2022 17.2+** or later + +If you're targeting older SDK versions or have team members on older tooling, CPM may cause build failures. + +### Multi-Repo Solutions + +If your solution spans multiple repositories that are built independently, CPM's single `Directory.Packages.props` won't help - each repo needs its own. + +--- + +## CLI Command Reference + +### Adding Packages + +```bash +# Add latest stable version +dotnet add package Serilog + +# Add specific version +dotnet add package Serilog --version 4.0.0 + +# Add prerelease +dotnet add package Serilog --prerelease + +# Add to specific project +dotnet add src/MyApp/MyApp.csproj package Serilog +``` + +### Removing Packages + +```bash +# Remove from current project +dotnet remove package Serilog + +# Remove from specific project +dotnet remove src/MyApp/MyApp.csproj package Serilog +``` + +### Listing Packages + +```bash +# List all packages in solution +dotnet list package + +# Show outdated packages +dotnet list package --outdated + +# Include transitive dependencies +dotnet list package --include-transitive + +# Show vulnerable packages +dotnet list package --vulnerable + +# Show deprecated packages +dotnet list package --deprecated +``` + +### Updating Packages + +```bash +# With CPM: Edit the version in Directory.Packages.props +# Then restore to apply +dotnet restore + +# Without CPM: Remove and add with new version +dotnet remove package Serilog +dotnet add package Serilog --version 4.1.0 + +# Or use dotnet-outdated tool (recommended) +dotnet tool install --global dotnet-outdated-tool +dotnet outdated --upgrade +``` + +### Restore and Clean + +```bash +# Restore packages +dotnet restore + +# Clear local cache (troubleshooting) +dotnet nuget locals all --clear + +# Force restore (ignore cache) +dotnet restore --force +``` + +--- + +## Package Sources + +### List Sources + +```bash +dotnet nuget list source +``` + +### Add Private Feed + +```bash +# Add authenticated feed +dotnet nuget add source https://pkgs.dev.azure.com/myorg/_packaging/myfeed/nuget/v3/index.json \ + --name MyFeed \ + --username az \ + --password $PAT \ + --store-password-in-clear-text +``` + +### NuGet.config + +For solution-specific sources, create `NuGet.config`: + +```xml + + + + + + + + + + + + + + +``` + +--- + +## Common Patterns + +### Development-Only Packages + +```xml + + + + + +``` + +### Conditional Packages + +```xml + + + + + + + + + +``` + +### Version Override (Escape Hatch) + +When you must override CPM for one project (rare): + +```xml + + +``` + +**Warning**: This is detected by Slopwatch (see `dotnet/slopwatch` skill) as potential slop. + +--- + +## Troubleshooting + +### Version Conflicts + +```bash +# See full dependency tree +dotnet list package --include-transitive + +# Find what's pulling in a specific package +dotnet list package --include-transitive | grep -i "PackageName" +``` + +### Restore Failures + +```bash +# Clear all caches +dotnet nuget locals all --clear + +# Restore with detailed logging +dotnet restore --verbosity detailed + +# Check for locked packages +cat packages.lock.json +``` + +### Lock Files + +For reproducible builds, use package lock files: + +```xml + + + true + +``` + +Then commit `packages.lock.json` files. + +--- + +## Anti-Patterns + +### Don't: Edit XML Directly + +```xml + + + +``` + +### Don't: Inline Versions with CPM + +```xml + + + + + +``` + +### Don't: Mix Version Management + +```xml + + + +``` + +### Don't: Forget Shared Variables + +```xml + + + + + + + +``` + +--- + +## Quick Reference + +| Task | Command | +|------|---------| +| Add package | `dotnet add package ` | +| Add specific version | `dotnet add package --version ` | +| Remove package | `dotnet remove package ` | +| List packages | `dotnet list package` | +| Show outdated | `dotnet list package --outdated` | +| Show vulnerable | `dotnet list package --vulnerable` | +| Restore | `dotnet restore` | +| Clear cache | `dotnet nuget locals all --clear` | + +--- + +## Resources + +- **Central Package Management**: https://learn.microsoft.com/en-us/nuget/consume-packages/central-package-management +- **dotnet CLI Reference**: https://learn.microsoft.com/en-us/dotnet/core/tools/ +- **NuGet.config Reference**: https://learn.microsoft.com/en-us/nuget/reference/nuget-config-file diff --git a/.github/skills/project-structure/SKILL.md b/.github/skills/project-structure/SKILL.md new file mode 100644 index 00000000..944a3bbb --- /dev/null +++ b/.github/skills/project-structure/SKILL.md @@ -0,0 +1,521 @@ +--- +name: dotnet-project-structure +description: Modern .NET project structure including .slnx solution format, Directory.Build.props, central package management, SourceLink, version management with RELEASE_NOTES.md, and SDK pinning with global.json. +invocable: false +--- + +# .NET Project Structure and Build Configuration + +## When to Use This Skill + +Use this skill when: +- Setting up a new .NET solution with modern best practices +- Configuring centralized build properties across multiple projects +- Implementing central package version management +- Setting up SourceLink for debugging and NuGet packages +- Automating version management with release notes +- Pinning SDK versions for consistent builds + +## Related Skills + +- **`dotnet-local-tools`** - Managing local .NET tools with dotnet-tools.json +- **`microsoft-extensions-configuration`** - Configuration validation patterns + +--- + +## Solution File Format (.slnx) + +The `.slnx` format is the modern XML-based solution file format introduced in .NET 9. It replaces the traditional `.sln` format. + +### Benefits Over Traditional .sln + +| Aspect | .sln (Legacy) | .slnx (Modern) | +|--------|---------------|----------------| +| Format | Custom text format | Standard XML | +| Readability | GUIDs, cryptic syntax | Clean, human-readable | +| Version control | Hard to diff/merge | Easy to diff/merge | +| Editing | IDE required | Any text editor | + +### Version Requirements + +| Tool | Minimum Version | +|------|-----------------| +| .NET SDK | 9.0.200 | +| Visual Studio | 17.13 | +| MSBuild | Visual Studio Build Tools 17.13 | + +**Note:** Starting with .NET 10, `dotnet new sln` creates `.slnx` files by default. In .NET 9, you must explicitly migrate or specify the format. + +### Example .slnx File + +```xml + + + + + + + + + + + + + + + + +``` + +### Migrating from .sln to .slnx + +Use the `dotnet sln migrate` command to convert existing solutions: + +```bash +# Migrate a specific solution file +dotnet sln MySolution.sln migrate + +# Or if only one .sln exists in the directory, just run: +dotnet sln migrate +``` + +**Important:** Do not keep both `.sln` and `.slnx` files in the same repository. This causes issues with automatic solution detection and can lead to sync problems. After migration, delete the old `.sln` file. + +You can also migrate in Visual Studio: +1. Open the solution +2. Select the Solution in Solution Explorer +3. Go to **File > Save Solution As...** +4. Change "Save as type" to **Xml Solution File (*.slnx)** + +### Creating a New .slnx Solution + +```bash +# .NET 10+: Creates .slnx by default +dotnet new sln --name MySolution + +# .NET 9: Specify the format explicitly +dotnet new sln --name MySolution --format slnx + +# Add projects (works the same for both formats) +dotnet sln add src/MyApp/MyApp.csproj +``` + +### Recommendation + +**If you're using .NET 9.0.200 or later, migrate your solutions to .slnx.** The benefits are significant: +- Dramatically fewer merge conflicts (no random GUIDs changing) +- Human-readable and editable in any text editor +- Consistent with modern `.csproj` format +- Better diff/review experience in pull requests + +--- + +## Directory.Build.props + +`Directory.Build.props` provides centralized build configuration that applies to all projects in a directory tree. Place it at the solution root. + +### Complete Example + +```xml + + + + Your Team + Your Company + + Copyright © 2020-$([System.DateTime]::Now.Year) Your Company + Your Product + https://github.com/yourorg/yourrepo + https://github.com/yourorg/yourrepo + Apache-2.0 + your;tags;here + + + + + latest + enable + enable + true + $(NoWarn);CS1591 + + + + + 1.0.0 + See RELEASE_NOTES.md + + + + + netstandard2.0 + net8.0 + net9.0 + + + + + true + true + true + snupkg + + + + + + + + + + + + + + logo.png + README.md + + + + + + + +``` + +### Key Patterns + +#### Dynamic Copyright Year + +```xml +Copyright © 2020-$([System.DateTime]::Now.Year) Your Company +``` + +Uses MSBuild property functions to insert current year at build time. No manual updates needed. + +#### Reusable Target Framework Properties + +Define target frameworks once, reference everywhere: + +```xml + + + net8.0 + net9.0 + + + + + $(NetLibVersion) + + + + + $(NetTestVersion) + +``` + +#### SourceLink for NuGet Packages + +SourceLink enables step-through debugging of NuGet packages: + +```xml + + true + true + true + snupkg + + + + + + + + + +``` + +--- + +## Directory.Packages.props - Central Package Management + +Central Package Management (CPM) provides a single source of truth for all NuGet package versions. + +### Setup + +```xml + + + true + + + + + 1.5.35 + 9.1.0 + + + + + + + + + + + + + + + + + + + + + + + + +``` + +### Consuming Packages (No Version Needed) + +```xml + + + + + + + + + + + + + +``` + +### Benefits + +1. **Single source of truth** - All versions in one file +2. **No version drift** - All projects use same versions +3. **Easy updates** - Change once, applies everywhere +4. **Grouped packages** - Version variables for related packages (e.g., all Akka packages) + +--- + +## global.json - SDK Version Pinning + +Pin the .NET SDK version for consistent builds across all environments. + +```json +{ + "sdk": { + "version": "9.0.200", + "rollForward": "latestFeature" + } +} +``` + +### Roll Forward Policies + +| Policy | Behavior | +|--------|----------| +| `disable` | Exact version required | +| `patch` | Same major.minor, latest patch | +| `feature` | Same major, latest minor.patch | +| `latestFeature` | Same major, latest feature band | +| `minor` | Same major, latest minor | +| `latestMinor` | Same major, latest minor | +| `major` | Latest SDK (not recommended) | + +**Recommended:** `latestFeature` - Allows patch updates within the same feature band. + +--- + +## Version Management with RELEASE_NOTES.md + +### Release Notes Format + +```markdown +#### 1.2.0 January 15th 2025 #### + +- Added new feature X +- Fixed bug in Y +- Improved performance of Z + +#### 1.1.0 December 10th 2024 #### + +- Initial release with features A, B, C +``` + +### Parsing Script (getReleaseNotes.ps1) + +```powershell +function Get-ReleaseNotes { + param ( + [Parameter(Mandatory=$true)] + [string]$MarkdownFile + ) + + $content = Get-Content -Path $MarkdownFile -Raw + $sections = $content -split "####" + + $result = [PSCustomObject]@{ + Version = $null + Date = $null + ReleaseNotes = $null + } + + if ($sections.Count -ge 3) { + $header = $sections[1].Trim() + $releaseNotes = $sections[2].Trim() + + $headerParts = $header -split " ", 2 + if ($headerParts.Count -eq 2) { + $result.Version = $headerParts[0] + $result.Date = $headerParts[1] + } + + $result.ReleaseNotes = $releaseNotes + } + + return $result +} +``` + +### Version Bump Script (bumpVersion.ps1) + +```powershell +function UpdateVersionAndReleaseNotes { + param ( + [Parameter(Mandatory=$true)] + [PSCustomObject]$ReleaseNotesResult, + [Parameter(Mandatory=$true)] + [string]$XmlFilePath + ) + + $xmlContent = New-Object XML + $xmlContent.Load($XmlFilePath) + + # Update VersionPrefix + $versionElement = $xmlContent.SelectSingleNode("//VersionPrefix") + $versionElement.InnerText = $ReleaseNotesResult.Version + + # Update PackageReleaseNotes + $notesElement = $xmlContent.SelectSingleNode("//PackageReleaseNotes") + $notesElement.InnerText = $ReleaseNotesResult.ReleaseNotes + + $xmlContent.Save($XmlFilePath) +} +``` + +### Build Script (build.ps1) + +```powershell +# Load helper scripts +. "$PSScriptRoot\scripts\getReleaseNotes.ps1" +. "$PSScriptRoot\scripts\bumpVersion.ps1" + +# Parse release notes and update Directory.Build.props +$releaseNotes = Get-ReleaseNotes -MarkdownFile (Join-Path -Path $PSScriptRoot -ChildPath "RELEASE_NOTES.md") +UpdateVersionAndReleaseNotes -ReleaseNotesResult $releaseNotes -XmlFilePath (Join-Path -Path $PSScriptRoot -ChildPath "Directory.Build.props") + +Write-Output "Updated to version $($releaseNotes.Version)" +``` + +### CI/CD Integration + +```yaml +# GitHub Actions example +- name: Update version from release notes + shell: pwsh + run: ./build.ps1 + +- name: Build + run: dotnet build -c Release + +- name: Pack with tag version + run: dotnet pack -c Release /p:PackageVersion=${{ github.ref_name }} + +- name: Push to NuGet + run: dotnet nuget push **/*.nupkg --api-key ${{ secrets.NUGET_API_KEY }} --source https://api.nuget.org/v3/index.json +``` + +--- + +## NuGet.Config + +Configure NuGet sources and behavior: + +```xml + + + + + + + + + + + + + +``` + +**Key Settings:** +- `` - Remove inherited/default sources for reproducible builds +- `disableSourceControlIntegration` - Prevents TFS/Git integration issues + +--- + +## Complete Project Structure + +``` +MySolution/ +├── .config/ +│ └── dotnet-tools.json # Local .NET tools +├── .github/ +│ └── workflows/ +│ ├── pr-validation.yml # PR checks +│ └── release.yml # NuGet publishing +├── scripts/ +│ ├── getReleaseNotes.ps1 # Parse RELEASE_NOTES.md +│ └── bumpVersion.ps1 # Update Directory.Build.props +├── src/ +│ ├── MyApp/ +│ │ └── MyApp.csproj +│ └── MyApp.Core/ +│ └── MyApp.Core.csproj +├── tests/ +│ └── MyApp.Tests/ +│ └── MyApp.Tests.csproj +├── Directory.Build.props # Centralized build config +├── Directory.Packages.props # Central package versions +├── MySolution.slnx # Modern solution file +├── global.json # SDK version pinning +├── NuGet.Config # Package source config +├── build.ps1 # Build orchestration +├── RELEASE_NOTES.md # Version history +├── README.md # Project documentation +└── logo.png # Package icon +``` + +--- + +## Quick Reference + +| File | Purpose | +|------|---------| +| `MySolution.slnx` | Modern XML solution file | +| `Directory.Build.props` | Centralized build properties | +| `Directory.Packages.props` | Central package version management | +| `global.json` | SDK version pinning | +| `NuGet.Config` | Package source configuration | +| `RELEASE_NOTES.md` | Version history (parsed by build) | +| `build.ps1` | Build orchestration script | +| `.config/dotnet-tools.json` | Local .NET tools | diff --git a/.github/skills/serialization/SKILL.md b/.github/skills/serialization/SKILL.md new file mode 100644 index 00000000..00abcfc1 --- /dev/null +++ b/.github/skills/serialization/SKILL.md @@ -0,0 +1,413 @@ +--- +name: serialization +description: Choose the right serialization format for .NET applications. Prefer schema-based formats (Protobuf, MessagePack) over reflection-based (Newtonsoft.Json). Use System.Text.Json with AOT source generators for JSON scenarios. +invocable: false +--- + +# Serialization in .NET + +## When to Use This Skill + +Use this skill when: +- Choosing a serialization format for APIs, messaging, or persistence +- Migrating from Newtonsoft.Json to System.Text.Json +- Implementing AOT-compatible serialization +- Designing wire formats for distributed systems +- Optimizing serialization performance + +--- + +## Schema-Based vs Reflection-Based + +| Aspect | Schema-Based | Reflection-Based | +|--------|--------------|------------------| +| **Examples** | Protobuf, MessagePack, System.Text.Json (source gen) | Newtonsoft.Json, BinaryFormatter | +| **Type info in payload** | No (external schema) | Yes (type names embedded) | +| **Versioning** | Explicit field numbers/names | Implicit (type structure) | +| **Performance** | Fast (no reflection) | Slower (runtime reflection) | +| **AOT compatible** | Yes | No | +| **Wire compatibility** | Excellent | Poor | + +**Recommendation**: Use schema-based serialization for anything that crosses process boundaries. + +--- + +## Format Recommendations + +| Use Case | Recommended Format | Why | +|----------|-------------------|-----| +| **REST APIs** | System.Text.Json (source gen) | Standard, AOT-compatible | +| **gRPC** | Protocol Buffers | Native format, excellent versioning | +| **Actor messaging** | MessagePack or Protobuf | Compact, fast, version-safe | +| **Event sourcing** | Protobuf or MessagePack | Must handle old events forever | +| **Caching** | MessagePack | Compact, fast | +| **Configuration** | JSON (System.Text.Json) | Human-readable | +| **Logging** | JSON (System.Text.Json) | Structured, parseable | + +### Formats to Avoid + +| Format | Problem | +|--------|---------| +| **BinaryFormatter** | Security vulnerabilities, deprecated, never use | +| **Newtonsoft.Json default** | Type names in payload break on rename | +| **DataContractSerializer** | Complex, poor versioning | +| **XML** | Verbose, slow, complex | + +--- + +## System.Text.Json with Source Generators + +For JSON serialization, use System.Text.Json with source generators for AOT compatibility and performance. + +### Setup + +```csharp +// Define a JsonSerializerContext with all your types +[JsonSerializable(typeof(Order))] +[JsonSerializable(typeof(OrderItem))] +[JsonSerializable(typeof(Customer))] +[JsonSerializable(typeof(List))] +[JsonSourceGenerationOptions( + PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase, + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull)] +public partial class AppJsonContext : JsonSerializerContext { } +``` + +### Usage + +```csharp +// Serialize with context +var json = JsonSerializer.Serialize(order, AppJsonContext.Default.Order); + +// Deserialize with context +var order = JsonSerializer.Deserialize(json, AppJsonContext.Default.Order); + +// Configure in ASP.NET Core +builder.Services.ConfigureHttpJsonOptions(options => +{ + options.SerializerOptions.TypeInfoResolverChain.Insert(0, AppJsonContext.Default); +}); +``` + +### Benefits + +- **No reflection at runtime** - All type info generated at compile time +- **AOT compatible** - Works with Native AOT publishing +- **Faster** - No runtime type analysis +- **Trim-safe** - Linker knows exactly what's needed + +--- + +## Protocol Buffers (Protobuf) + +Best for: Actor systems, gRPC, event sourcing, any long-lived wire format. + +### Setup + +```bash +dotnet add package Google.Protobuf +dotnet add package Grpc.Tools +``` + +### Define Schema + +```protobuf +// orders.proto +syntax = "proto3"; + +message Order { + string id = 1; + string customer_id = 2; + repeated OrderItem items = 3; + int64 created_at_ticks = 4; + + // Adding new fields is always safe + string notes = 5; // Added in v2 - old readers ignore it +} + +message OrderItem { + string product_id = 1; + int32 quantity = 2; + int64 price_cents = 3; +} +``` + +### Versioning Rules + +```protobuf +// SAFE: Add new fields with new numbers +message Order { + string id = 1; + string customer_id = 2; + string shipping_address = 5; // NEW - safe +} + +// SAFE: Remove fields (old readers ignore unknown, new readers use default) +// Just stop using the field, keep the number reserved +message Order { + string id = 1; + // customer_id removed, but field 2 is reserved + reserved 2; +} + +// UNSAFE: Change field types +message Order { + int32 id = 1; // Was: string - BREAKS! +} + +// UNSAFE: Reuse field numbers +message Order { + reserved 2; + string new_field = 2; // Reusing 2 - BREAKS! +} +``` + +--- + +## MessagePack + +Best for: High-performance scenarios, compact payloads, actor messaging. + +### Setup + +```bash +dotnet add package MessagePack +dotnet add package MessagePack.Annotations +``` + +### Usage with Contracts + +```csharp +[MessagePackObject] +public sealed class Order +{ + [Key(0)] + public required string Id { get; init; } + + [Key(1)] + public required string CustomerId { get; init; } + + [Key(2)] + public required IReadOnlyList Items { get; init; } + + [Key(3)] + public required DateTimeOffset CreatedAt { get; init; } + + // New field - old readers skip unknown keys + [Key(4)] + public string? Notes { get; init; } +} + +// Serialize +var bytes = MessagePackSerializer.Serialize(order); + +// Deserialize +var order = MessagePackSerializer.Deserialize(bytes); +``` + +### AOT-Compatible Setup + +```csharp +// Use source generator for AOT +[MessagePackObject] +public partial class Order { } // partial enables source gen + +// Configure resolver +var options = MessagePackSerializerOptions.Standard + .WithResolver(CompositeResolver.Create( + GeneratedResolver.Instance, // Generated + StandardResolver.Instance)); +``` + +--- + +## Migrating from Newtonsoft.Json + +### Common Issues + +| Newtonsoft | System.Text.Json | Fix | +|------------|------------------|-----| +| `$type` in JSON | Not supported by default | Use discriminators or custom converters | +| `JsonProperty` | `JsonPropertyName` | Different attribute | +| `DefaultValueHandling` | `DefaultIgnoreCondition` | Different API | +| `NullValueHandling` | `DefaultIgnoreCondition` | Different API | +| Private setters | Requires `[JsonInclude]` | Explicit opt-in | +| Polymorphism | `[JsonDerivedType]` (.NET 7+) | Explicit discriminators | + +### Migration Pattern + +```csharp +// Newtonsoft (reflection-based) +public class Order +{ + [JsonProperty("order_id")] + public string Id { get; set; } + + [JsonProperty(NullValueHandling = NullValueHandling.Ignore)] + public string? Notes { get; set; } +} + +// System.Text.Json (source-gen compatible) +public sealed record Order( + [property: JsonPropertyName("order_id")] + string Id, + + string? Notes // Null handling via JsonSerializerOptions +); + +[JsonSerializable(typeof(Order))] +[JsonSourceGenerationOptions( + PropertyNamingPolicy = JsonKnownNamingPolicy.SnakeCaseLower, + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull)] +public partial class OrderJsonContext : JsonSerializerContext { } +``` + +### Polymorphism with Discriminators + +```csharp +// .NET 7+ polymorphism +[JsonDerivedType(typeof(CreditCardPayment), "credit_card")] +[JsonDerivedType(typeof(BankTransferPayment), "bank_transfer")] +public abstract record Payment(decimal Amount); + +public sealed record CreditCardPayment(decimal Amount, string Last4) : Payment(Amount); +public sealed record BankTransferPayment(decimal Amount, string AccountNumber) : Payment(Amount); + +// Serializes as: +// { "$type": "credit_card", "amount": 100, "last4": "1234" } +``` + +--- + +## Wire Compatibility Patterns + +### Tolerant Reader + +Old code must safely ignore unknown fields: + +```csharp +// Protobuf/MessagePack: Automatic - unknown fields skipped +// System.Text.Json: Configure to allow +var options = new JsonSerializerOptions +{ + UnmappedMemberHandling = JsonUnmappedMemberHandling.Skip +}; +``` + +### Introduce Read Before Write + +Deploy deserializers before serializers for new formats: + +```csharp +// Phase 1: Add deserializer (deployed everywhere) +public Order Deserialize(byte[] data, string manifest) => manifest switch +{ + "Order.V1" => DeserializeV1(data), + "Order.V2" => DeserializeV2(data), // NEW - can read V2 + _ => throw new NotSupportedException() +}; + +// Phase 2: Enable serializer (next release, after V1 deployed everywhere) +public (byte[] data, string manifest) Serialize(Order order) => + _useV2Format + ? (SerializeV2(order), "Order.V2") + : (SerializeV1(order), "Order.V1"); +``` + +### Never Embed Type Names + +```csharp +// BAD: Type name in payload - renaming class breaks wire format +{ + "$type": "MyApp.Order, MyApp.Core", + "id": "123" +} + +// GOOD: Explicit discriminator - refactoring safe +{ + "type": "order", + "id": "123" +} +``` + +--- + +## Performance Comparison + +Approximate throughput (higher is better): + +| Format | Serialize | Deserialize | Size | +|--------|-----------|-------------|------| +| MessagePack | ★★★★★ | ★★★★★ | ★★★★★ | +| Protobuf | ★★★★★ | ★★★★★ | ★★★★★ | +| System.Text.Json (source gen) | ★★★★☆ | ★★★★☆ | ★★★☆☆ | +| System.Text.Json (reflection) | ★★★☆☆ | ★★★☆☆ | ★★★☆☆ | +| Newtonsoft.Json | ★★☆☆☆ | ★★☆☆☆ | ★★★☆☆ | + +For hot paths, prefer MessagePack or Protobuf. + +--- + +## Akka.NET Serialization + +For Akka.NET actor systems, use schema-based serialization: + +```hocon +akka { + actor { + serializers { + messagepack = "Akka.Serialization.MessagePackSerializer, Akka.Serialization.MessagePack" + } + serialization-bindings { + "MyApp.Messages.IMessage, MyApp" = messagepack + } + } +} +``` + +See [Akka.NET Serialization Docs](https://getakka.net/articles/networking/serialization.html). + +--- + +## Best Practices + +### DO + +```csharp +// Use source generators for System.Text.Json +[JsonSerializable(typeof(Order))] +public partial class AppJsonContext : JsonSerializerContext { } + +// Use explicit field numbers/keys +[MessagePackObject] +public class Order +{ + [Key(0)] public string Id { get; init; } +} + +// Use records for immutable message types +public sealed record OrderCreated(OrderId Id, CustomerId CustomerId); +``` + +### DON'T + +```csharp +// Don't use BinaryFormatter (ever) +var formatter = new BinaryFormatter(); // Security risk! + +// Don't embed type names in wire format +settings.TypeNameHandling = TypeNameHandling.All; // Breaks on rename! + +// Don't use reflection serialization for hot paths +JsonConvert.SerializeObject(order); // Slow, not AOT-compatible +``` + +--- + +## Resources + +- **System.Text.Json Source Generation**: https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/source-generation +- **Protocol Buffers**: https://protobuf.dev/ +- **MessagePack-CSharp**: https://github.com/MessagePack-CSharp/MessagePack-CSharp +- **Akka.NET Serialization**: https://getakka.net/articles/networking/serialization.html +- **Wire Compatibility**: https://getakka.net/community/contributing/wire-compatibility.html diff --git a/.github/skills/slopwatch/SKILL.md b/.github/skills/slopwatch/SKILL.md new file mode 100644 index 00000000..ce89649a --- /dev/null +++ b/.github/skills/slopwatch/SKILL.md @@ -0,0 +1,318 @@ +--- +name: dotnet-slopwatch +description: Use Slopwatch to detect LLM reward hacking in .NET code changes. Run after every code modification to catch disabled tests, suppressed warnings, empty catch blocks, and other shortcuts that mask real problems. +invocable: true +--- + +# Slopwatch: LLM Anti-Cheat for .NET + +## When to Use This Skill + +**Use this skill constantly.** Every time an LLM (including Claude) makes changes to: +- C# source files (.cs) +- Project files (.csproj) +- Props files (Directory.Build.props, Directory.Packages.props) +- Test files + +Run slopwatch to validate the changes don't introduce "slop." + +## What is Slop? + +"Slop" refers to shortcuts LLMs take that make tests pass or builds succeed without actually solving the underlying problem. These are reward hacking behaviors - the LLM optimizes for apparent success rather than real fixes. + +### Common Slop Patterns + +| Pattern | Example | Why It's Bad | +|---------|---------|--------------| +| Disabled tests | `[Fact(Skip="flaky")]` | Hides failures instead of fixing them | +| Warning suppression | `#pragma warning disable CS8618` | Silences compiler without fixing issue | +| Empty catch blocks | `catch (Exception) { }` | Swallows errors, hides bugs | +| Arbitrary delays | `await Task.Delay(1000);` | Masks race conditions, makes tests slow | +| Project-level suppression | `CS1591` | Disables warnings project-wide | +| CPM bypass | `Version="1.0.0"` inline | Undermines central package management | + +**Never accept these patterns.** If an LLM introduces slop, reject the change and require a proper fix. + +--- + +## Installation + +### As a Local Tool (Recommended) + +Add to `.config/dotnet-tools.json`: + +```json +{ + "version": 1, + "isRoot": true, + "tools": { + "slopwatch.cmd": { + "version": "0.2.0", + "commands": ["slopwatch"], + "rollForward": false + } + } +} +``` + +Then restore: +```bash +dotnet tool restore +``` + +### As a Global Tool + +```bash +dotnet tool install --global Slopwatch.Cmd +``` + +--- + +## First-Time Setup: Establish a Baseline + +Before using slopwatch on an existing project, create a baseline of current issues: + +```bash +# Initialize baseline from existing code +slopwatch init + +# This creates .slopwatch/baseline.json +git add .slopwatch/baseline.json +git commit -m "Add slopwatch baseline" +``` + +**Why baseline?** Legacy code may have existing issues. The baseline ensures slopwatch only catches **new** slop being introduced, not pre-existing technical debt. + +--- + +## Usage During LLM Sessions + +### After Every Code Change + +Run slopwatch after any LLM-generated code modification: + +```bash +# Analyze for new issues (uses baseline) +slopwatch analyze + +# Use strict mode - fail on warnings too +slopwatch analyze --fail-on warning +``` + +### When Slopwatch Flags an Issue + +**Do not ignore it.** Instead: + +1. **Understand why** the LLM took the shortcut +2. **Request a proper fix** - be specific about what's wrong +3. **Verify the fix** doesn't introduce different slop + +``` +# Example: LLM disabled a test +❌ SW001 [Error]: Disabled test detected + File: tests/MyApp.Tests/OrderTests.cs:45 + Pattern: [Fact(Skip="Test is flaky")] + +# Correct response: Ask for actual fix +"This test was disabled instead of fixed. Please investigate why +it's flaky and fix the underlying timing/race condition issue." +``` + +### Updating the Baseline (Rare) + +Only update the baseline when slop is **truly justified** and documented: + +```bash +# Add current detections to baseline (use sparingly!) +slopwatch analyze --update-baseline +``` + +**Justification examples:** +- Third-party library forces a pattern (e.g., must suppress specific warning) +- Intentional delay for rate limiting (not test flakiness) +- Generated code that can't be modified + +Document why in a code comment when updating baseline. + +--- + +## Claude Code Hook Integration + +Add slopwatch as a hook to automatically validate every edit. Create or update `.claude/settings.json`: + +```json +{ + "hooks": { + "PostToolUse": [ + { + "matcher": "Write|Edit|MultiEdit", + "hooks": [ + { + "type": "command", + "command": "slopwatch analyze -d . --hook", + "timeout": 60000 + } + ] + } + ] + } +} +``` + +The `--hook` flag: +- Only analyzes **git dirty files** (fast, even on large repos) +- Outputs errors to stderr in readable format +- Blocks the edit on warnings/errors (exit code 2) +- Claude sees the error and can fix it immediately + +--- + +## CI/CD Integration + +Add slopwatch to your CI pipeline as a quality gate: + +### GitHub Actions + +```yaml +jobs: + slopwatch: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Setup .NET + uses: actions/setup-dotnet@v4 + with: + dotnet-version: '9.0.x' + + - name: Install Slopwatch + run: dotnet tool install --global Slopwatch.Cmd + + - name: Run Slopwatch + run: slopwatch analyze -d . --fail-on warning +``` + +### Azure Pipelines + +```yaml +- task: DotNetCoreCLI@2 + displayName: 'Install Slopwatch' + inputs: + command: 'custom' + custom: 'tool' + arguments: 'install --global Slopwatch.Cmd' + +- script: slopwatch analyze -d . --fail-on warning + displayName: 'Slopwatch Analysis' +``` + +--- + +## Detection Rules + +| Rule | Severity | What It Catches | +|------|----------|-----------------| +| SW001 | Error | Disabled tests (`Skip=`, `Ignore`, `#if false`) | +| SW002 | Warning | Warning suppression (`#pragma warning disable`, `SuppressMessage`) | +| SW003 | Error | Empty catch blocks that swallow exceptions | +| SW004 | Warning | Arbitrary delays in tests (`Task.Delay`, `Thread.Sleep`) | +| SW005 | Warning | Project file slop (`NoWarn`, `TreatWarningsAsErrors=false`) | +| SW006 | Warning | CPM bypass (`VersionOverride`, inline `Version` attributes) | + +--- + +## Configuration + +Create `.slopwatch/slopwatch.json` to customize: + +```json +{ + "minSeverity": "warning", + "rules": { + "SW001": { "enabled": true, "severity": "error" }, + "SW002": { "enabled": true, "severity": "warning" }, + "SW003": { "enabled": true, "severity": "error" }, + "SW004": { "enabled": true, "severity": "warning" }, + "SW005": { "enabled": true, "severity": "warning" }, + "SW006": { "enabled": true, "severity": "warning" } + }, + "exclude": [ + "**/Generated/**", + "**/obj/**", + "**/bin/**" + ] +} +``` + +### Strict Mode (Recommended for LLM Sessions) + +For maximum protection during LLM coding sessions, elevate all rules to errors: + +```json +{ + "minSeverity": "warning", + "rules": { + "SW001": { "enabled": true, "severity": "error" }, + "SW002": { "enabled": true, "severity": "error" }, + "SW003": { "enabled": true, "severity": "error" }, + "SW004": { "enabled": true, "severity": "error" }, + "SW005": { "enabled": true, "severity": "error" }, + "SW006": { "enabled": true, "severity": "error" } + } +} +``` + +--- + +## The Philosophy: Zero Tolerance for New Slop + +1. **Baseline captures legacy** - Existing issues are acknowledged but isolated +2. **New slop is blocked** - Any new shortcut fails the build/edit +3. **Exceptions require justification** - If you must update baseline, document why +4. **LLMs are not special** - The same rules apply to human and AI-generated code + +The goal is to prevent the gradual accumulation of technical debt that occurs when LLMs optimize for "make the test pass" rather than "fix the actual problem." + +--- + +## Quick Reference + +```bash +# First time setup +slopwatch init +git add .slopwatch/baseline.json + +# After every LLM code change +slopwatch analyze + +# Strict mode (recommended) +slopwatch analyze --fail-on warning + +# With stats (performance debugging) +slopwatch analyze --stats + +# Update baseline (rare, document why) +slopwatch analyze --update-baseline + +# JSON output for tooling +slopwatch analyze --output json +``` + +--- + +## When to Override (Almost Never) + +The only valid reasons to update baseline or disable a rule: + +| Scenario | Action | Required | +|----------|--------|----------| +| Third-party forces pattern | Update baseline | Code comment explaining why | +| Generated code (not editable) | Add to exclude list | Document in config | +| Intentional rate limiting delay | Update baseline | Code comment, not in test | +| Legacy code cleanup | One-time baseline update | PR description | + +**Invalid reasons:** +- "The test is flaky" → Fix the flakiness +- "The warning is annoying" → Fix the code +- "It works on my machine" → Fix the race condition +- "We'll fix it later" → Fix it now diff --git a/.github/skills/testcontainers/SKILL.md b/.github/skills/testcontainers/SKILL.md new file mode 100644 index 00000000..c4242827 --- /dev/null +++ b/.github/skills/testcontainers/SKILL.md @@ -0,0 +1,225 @@ +--- +name: testcontainers-integration-tests +description: Write integration tests using TestContainers for .NET with xUnit. Covers infrastructure testing with real databases, message queues, and caches in Docker containers instead of mocks. +invocable: false +--- + +# Integration Testing with TestContainers + +## When to Use This Skill + +Use this skill when: +- Writing integration tests that need real infrastructure (databases, caches, message queues) +- Testing data access layers against actual databases +- Verifying message queue integrations +- Testing Redis caching behavior +- Avoiding mocks for infrastructure components +- Ensuring tests work against production-like environments +- Testing database migrations and schema changes + +## Reference Files + +- [database-patterns.md](database-patterns.md): SQL Server, PostgreSQL, and migration testing examples +- [infrastructure-patterns.md](infrastructure-patterns.md): Redis, RabbitMQ, multi-container networks, container reuse, and Respawn + +## Core Principles + +1. **Real Infrastructure Over Mocks** - Use actual databases/services in containers, not mocks +2. **Test Isolation** - Each test gets fresh containers or fresh data +3. **Automatic Cleanup** - TestContainers handles container lifecycle and cleanup +4. **Fast Startup** - Reuse containers across tests in the same class when appropriate +5. **CI/CD Compatible** - Works seamlessly in Docker-enabled CI environments +6. **Port Randomization** - Containers use random ports to avoid conflicts + +## Why TestContainers Over Mocks? + +### The Problem with Mocking Infrastructure + +```csharp +// BAD: Mocking a database +public class OrderRepositoryTests +{ + private readonly Mock _mockDb = new(); + + [Fact] + public async Task GetOrder_ReturnsOrder() + { + // This doesn't test real SQL behavior, constraints, or performance + _mockDb.Setup(db => db.QueryAsync(It.IsAny())) + .ReturnsAsync(new[] { new Order { Id = 1 } }); + + var repo = new OrderRepository(_mockDb.Object); + var order = await repo.GetOrderAsync(1); + + Assert.NotNull(order); + } +} +``` + +Problems: doesn't test actual SQL queries, misses constraints/indexes, gives false confidence, doesn't catch SQL syntax errors. + +### Better: TestContainers with Real Database + +```csharp +// GOOD: Testing against a real database +public class OrderRepositoryTests : IAsyncLifetime +{ + private readonly TestcontainersContainer _dbContainer; + private IDbConnection _connection; + + public OrderRepositoryTests() + { + _dbContainer = new TestcontainersBuilder() + .WithImage("mcr.microsoft.com/mssql/server:2022-latest") + .WithEnvironment("ACCEPT_EULA", "Y") + .WithEnvironment("SA_PASSWORD", "Your_password123") + .WithPortBinding(1433, true) + .Build(); + } + + public async Task InitializeAsync() + { + await _dbContainer.StartAsync(); + var port = _dbContainer.GetMappedPublicPort(1433); + var connectionString = $"Server=localhost,{port};Database=TestDb;User Id=sa;Password=Your_password123;TrustServerCertificate=true"; + _connection = new SqlConnection(connectionString); + await _connection.OpenAsync(); + await RunMigrationsAsync(_connection); + } + + public async Task DisposeAsync() + { + await _connection.DisposeAsync(); + await _dbContainer.DisposeAsync(); + } + + [Fact] + public async Task GetOrder_WithRealDatabase_ReturnsOrder() + { + await _connection.ExecuteAsync( + "INSERT INTO Orders (Id, CustomerId, Total) VALUES (1, 'CUST1', 100.00)"); + + var repo = new OrderRepository(_connection); + var order = await repo.GetOrderAsync(1); + + Assert.NotNull(order); + Assert.Equal("CUST1", order.CustomerId); + Assert.Equal(100.00m, order.Total); + } +} +``` + +See [database-patterns.md](database-patterns.md) for complete SQL Server, PostgreSQL, and migration testing examples. + +See [infrastructure-patterns.md](infrastructure-patterns.md) for Redis, RabbitMQ, multi-container networks, container reuse, and Respawn database reset patterns. + +## Required NuGet Packages + +```xml + + + + + + + + + + + + + + +``` + +## Best Practices + +1. **Always Use IAsyncLifetime** - Proper async setup and teardown +2. **Wait for Port Availability** - Use `WaitStrategy` to ensure containers are ready +3. **Use Random Ports** - Let TestContainers assign ports automatically +4. **Clean Data Between Tests** - Either use fresh containers or truncate tables +5. **Reuse Containers When Possible** - Faster than creating new ones for each test +6. **Test Real Queries** - Don't just test mocks; verify actual SQL behavior +7. **Verify Constraints** - Test foreign keys, unique constraints, indexes +8. **Test Transactions** - Verify rollback and commit behavior +9. **Use Realistic Data** - Test with production-like data volumes +10. **Handle Cleanup** - Always dispose containers in `DisposeAsync` + +## Common Issues and Solutions + +### Container Startup Timeout + +```csharp +_container = new TestcontainersBuilder() + .WithImage("postgres:latest") + .WithWaitStrategy(Wait.ForUnixContainer() + .UntilPortIsAvailable(5432) + .WithTimeout(TimeSpan.FromMinutes(2))) + .Build(); +``` + +### Port Already in Use + +Always use random port mapping: +```csharp +.WithPortBinding(5432, true) // true = assign random public port +``` + +### Containers Not Cleaning Up + +Ensure proper disposal: +```csharp +public async Task DisposeAsync() +{ + await _connection?.DisposeAsync(); + await _container?.DisposeAsync(); +} +``` + +### Tests Fail in CI But Pass Locally + +Ensure CI has Docker support: +```yaml +# GitHub Actions +runs-on: ubuntu-latest # Has Docker pre-installed +``` + +## CI/CD Integration + +### GitHub Actions + +```yaml +name: Integration Tests + +on: [push, pull_request] + +jobs: + test: + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v3 + + - name: Setup .NET + uses: actions/setup-dotnet@v3 + with: + dotnet-version: 9.0.x + + - name: Run Integration Tests + run: | + dotnet test tests/YourApp.IntegrationTests \ + --filter Category=Integration \ + --logger trx + + - name: Cleanup Containers + if: always() + run: docker container prune -f +``` + +## Performance Tips + +1. **Reuse containers** - Share fixtures across tests in a collection +2. **Use Respawn** - Reset data without recreating containers +3. **Parallel execution** - TestContainers handles port conflicts automatically +4. **Use lightweight images** - Alpine versions are smaller and faster +5. **Cache images** - Docker will cache pulled images locally diff --git a/.github/skills/testcontainers/database-patterns.md b/.github/skills/testcontainers/database-patterns.md new file mode 100644 index 00000000..34cfe9b7 --- /dev/null +++ b/.github/skills/testcontainers/database-patterns.md @@ -0,0 +1,201 @@ +# Database Testing Patterns + +Full code examples for testing with SQL Server, PostgreSQL, and database migrations using TestContainers. + +## Contents + +- [SQL Server Integration Tests](#sql-server-integration-tests) +- [PostgreSQL Integration Tests](#postgresql-integration-tests) +- [Testing Migrations with Real Databases](#testing-migrations-with-real-databases) + +## SQL Server Integration Tests + +```csharp +using Testcontainers; +using Xunit; + +public class SqlServerTests : IAsyncLifetime +{ + private readonly TestcontainersContainer _dbContainer; + private IDbConnection _db; + + public SqlServerTests() + { + _dbContainer = new TestcontainersBuilder() + .WithImage("mcr.microsoft.com/mssql/server:2022-latest") + .WithEnvironment("ACCEPT_EULA", "Y") + .WithEnvironment("SA_PASSWORD", "Your_password123") + .WithPortBinding(1433, true) + .WithWaitStrategy(Wait.ForUnixContainer().UntilPortIsAvailable(1433)) + .Build(); + } + + public async Task InitializeAsync() + { + await _dbContainer.StartAsync(); + + var port = _dbContainer.GetMappedPublicPort(1433); + var connectionString = $"Server=localhost,{port};Database=master;User Id=sa;Password=Your_password123;TrustServerCertificate=true"; + + _db = new SqlConnection(connectionString); + await _db.OpenAsync(); + + // Create test database + await _db.ExecuteAsync("CREATE DATABASE TestDb"); + await _db.ExecuteAsync("USE TestDb"); + + // Run schema migrations + await _db.ExecuteAsync(@" + CREATE TABLE Orders ( + Id INT PRIMARY KEY, + CustomerId NVARCHAR(50) NOT NULL, + Total DECIMAL(18,2) NOT NULL, + CreatedAt DATETIME2 DEFAULT GETUTCDATE() + )"); + } + + public async Task DisposeAsync() + { + await _db.DisposeAsync(); + await _dbContainer.DisposeAsync(); + } + + [Fact] + public async Task CanInsertAndRetrieveOrder() + { + // Arrange + await _db.ExecuteAsync(@" + INSERT INTO Orders (Id, CustomerId, Total) + VALUES (1, 'CUST001', 99.99)"); + + // Act + var order = await _db.QuerySingleAsync( + "SELECT * FROM Orders WHERE Id = @Id", + new { Id = 1 }); + + // Assert + Assert.Equal(1, order.Id); + Assert.Equal("CUST001", order.CustomerId); + Assert.Equal(99.99m, order.Total); + } +} +``` + +## PostgreSQL Integration Tests + +```csharp +public class PostgreSqlTests : IAsyncLifetime +{ + private readonly TestcontainersContainer _dbContainer; + private NpgsqlConnection _connection; + + public PostgreSqlTests() + { + _dbContainer = new TestcontainersBuilder() + .WithImage("postgres:latest") + .WithEnvironment("POSTGRES_PASSWORD", "postgres") + .WithEnvironment("POSTGRES_DB", "testdb") + .WithPortBinding(5432, true) + .WithWaitStrategy(Wait.ForUnixContainer().UntilPortIsAvailable(5432)) + .Build(); + } + + public async Task InitializeAsync() + { + await _dbContainer.StartAsync(); + + var port = _dbContainer.GetMappedPublicPort(5432); + var connectionString = $"Host=localhost;Port={port};Database=testdb;Username=postgres;Password=postgres"; + + _connection = new NpgsqlConnection(connectionString); + await _connection.OpenAsync(); + + // Create schema + await _connection.ExecuteAsync(@" + CREATE TABLE orders ( + id SERIAL PRIMARY KEY, + customer_id VARCHAR(50) NOT NULL, + total NUMERIC(10,2) NOT NULL, + created_at TIMESTAMP DEFAULT NOW() + )"); + } + + public async Task DisposeAsync() + { + await _connection.DisposeAsync(); + await _dbContainer.DisposeAsync(); + } + + [Fact] + public async Task PostgreSql_ShouldHandleTransactions() + { + using var transaction = await _connection.BeginTransactionAsync(); + + await _connection.ExecuteAsync( + "INSERT INTO orders (customer_id, total) VALUES (@CustomerId, @Total)", + new { CustomerId = "CUST1", Total = 100.00m }, + transaction); + + await transaction.RollbackAsync(); + + var count = await _connection.QuerySingleAsync( + "SELECT COUNT(*) FROM orders"); + + Assert.Equal(0, count); // Rollback should prevent insert + } +} +``` + +## Testing Migrations with Real Databases + +```csharp +public class MigrationTests : IAsyncLifetime +{ + private readonly TestcontainersContainer _container; + private string _connectionString; + + public async Task InitializeAsync() + { + _container = new TestcontainersBuilder() + .WithImage("mcr.microsoft.com/mssql/server:2022-latest") + .WithEnvironment("ACCEPT_EULA", "Y") + .WithEnvironment("SA_PASSWORD", "Your_password123") + .WithPortBinding(1433, true) + .Build(); + + await _container.StartAsync(); + + var port = _container.GetMappedPublicPort(1433); + _connectionString = $"Server=localhost,{port};Database=TestDb;User Id=sa;Password=Your_password123;TrustServerCertificate=true"; + } + + [Fact] + public async Task Migrations_ShouldRunSuccessfully() + { + // Run Entity Framework migrations + var optionsBuilder = new DbContextOptionsBuilder(); + optionsBuilder.UseSqlServer(_connectionString); + + using var context = new AppDbContext(optionsBuilder.Options); + + // Apply migrations + await context.Database.MigrateAsync(); + + // Verify schema + var canConnect = await context.Database.CanConnectAsync(); + Assert.True(canConnect); + + // Verify tables exist + var tables = await context.Database.SqlQueryRaw( + "SELECT TABLE_NAME FROM INFORMATION_SCHEMA.TABLES").ToListAsync(); + + Assert.Contains("Orders", tables); + Assert.Contains("Customers", tables); + } + + public async Task DisposeAsync() + { + await _container.DisposeAsync(); + } +} +``` diff --git a/.github/skills/testcontainers/infrastructure-patterns.md b/.github/skills/testcontainers/infrastructure-patterns.md new file mode 100644 index 00000000..de33ff0f --- /dev/null +++ b/.github/skills/testcontainers/infrastructure-patterns.md @@ -0,0 +1,387 @@ +# Infrastructure Testing Patterns + +Patterns for testing Redis, RabbitMQ, multi-container networks, container reuse, and database reset with Respawn. + +## Contents + +- [Redis Integration Tests](#redis-integration-tests) +- [RabbitMQ Integration Tests](#rabbitmq-integration-tests) +- [Multi-Container Networks](#multi-container-networks) +- [Reusing Containers Across Tests](#reusing-containers-across-tests) +- [Database Reset with Respawn](#database-reset-with-respawn) + +## Redis Integration Tests + +```csharp +public class RedisTests : IAsyncLifetime +{ + private readonly TestcontainersContainer _redisContainer; + private IConnectionMultiplexer _redis; + + public RedisTests() + { + _redisContainer = new TestcontainersBuilder() + .WithImage("redis:alpine") + .WithPortBinding(6379, true) + .WithWaitStrategy(Wait.ForUnixContainer().UntilPortIsAvailable(6379)) + .Build(); + } + + public async Task InitializeAsync() + { + await _redisContainer.StartAsync(); + + var port = _redisContainer.GetMappedPublicPort(6379); + _redis = await ConnectionMultiplexer.ConnectAsync($"localhost:{port}"); + } + + public async Task DisposeAsync() + { + await _redis.DisposeAsync(); + await _redisContainer.DisposeAsync(); + } + + [Fact] + public async Task Redis_ShouldCacheValues() + { + var db = _redis.GetDatabase(); + + await db.StringSetAsync("key1", "value1"); + var value = await db.StringGetAsync("key1"); + + Assert.Equal("value1", value.ToString()); + } + + [Fact] + public async Task Redis_ShouldExpireKeys() + { + var db = _redis.GetDatabase(); + + await db.StringSetAsync("temp-key", "temp-value", + expiry: TimeSpan.FromSeconds(1)); + + Assert.True(await db.KeyExistsAsync("temp-key")); + + await Task.Delay(1100); + + Assert.False(await db.KeyExistsAsync("temp-key")); + } +} +``` + +## RabbitMQ Integration Tests + +```csharp +public class RabbitMqTests : IAsyncLifetime +{ + private readonly TestcontainersContainer _rabbitContainer; + private IConnection _connection; + + public RabbitMqTests() + { + _rabbitContainer = new TestcontainersBuilder() + .WithImage("rabbitmq:management-alpine") + .WithPortBinding(5672, true) + .WithPortBinding(15672, true) + .WithWaitStrategy(Wait.ForUnixContainer().UntilPortIsAvailable(5672)) + .Build(); + } + + public async Task InitializeAsync() + { + await _rabbitContainer.StartAsync(); + + var port = _rabbitContainer.GetMappedPublicPort(5672); + var factory = new ConnectionFactory + { + HostName = "localhost", + Port = port, + UserName = "guest", + Password = "guest" + }; + + _connection = await factory.CreateConnectionAsync(); + } + + public async Task DisposeAsync() + { + await _connection.CloseAsync(); + await _rabbitContainer.DisposeAsync(); + } + + [Fact] + public async Task RabbitMq_ShouldPublishAndConsumeMessage() + { + using var channel = await _connection.CreateChannelAsync(); + + var queueName = "test-queue"; + await channel.QueueDeclareAsync(queueName, durable: false, + exclusive: false, autoDelete: true); + + var message = "Hello, RabbitMQ!"; + var body = Encoding.UTF8.GetBytes(message); + await channel.BasicPublishAsync(exchange: "", + routingKey: queueName, + body: body); + + var consumer = new EventingBasicConsumer(channel); + var tcs = new TaskCompletionSource(); + + consumer.Received += (model, ea) => + { + var receivedMessage = Encoding.UTF8.GetString(ea.Body.ToArray()); + tcs.SetResult(receivedMessage); + }; + + await channel.BasicConsumeAsync(queueName, autoAck: true, + consumer: consumer); + + var received = await tcs.Task.WaitAsync(TimeSpan.FromSeconds(5)); + + Assert.Equal(message, received); + } +} +``` + +## Multi-Container Networks + +When you need multiple containers to communicate: + +```csharp +public class MultiContainerTests : IAsyncLifetime +{ + private readonly INetwork _network; + private readonly TestcontainersContainer _dbContainer; + private readonly TestcontainersContainer _redisContainer; + + public MultiContainerTests() + { + _network = new TestcontainersNetworkBuilder() + .Build(); + + _dbContainer = new TestcontainersBuilder() + .WithImage("postgres:latest") + .WithNetwork(_network) + .WithNetworkAliases("db") + .WithEnvironment("POSTGRES_PASSWORD", "postgres") + .Build(); + + _redisContainer = new TestcontainersBuilder() + .WithImage("redis:alpine") + .WithNetwork(_network) + .WithNetworkAliases("redis") + .Build(); + } + + public async Task InitializeAsync() + { + await _network.CreateAsync(); + await Task.WhenAll( + _dbContainer.StartAsync(), + _redisContainer.StartAsync()); + } + + public async Task DisposeAsync() + { + await Task.WhenAll( + _dbContainer.DisposeAsync().AsTask(), + _redisContainer.DisposeAsync().AsTask()); + await _network.DisposeAsync(); + } + + [Fact] + public async Task Containers_CanCommunicate() + { + // Both containers can reach each other via network aliases + // db -> redis://redis:6379 + // redis -> postgres://db:5432 + } +} +``` + +## Reusing Containers Across Tests + +For faster test execution, reuse containers across tests in a class: + +```csharp +[Collection("Database collection")] +public class FastDatabaseTests +{ + private readonly DatabaseFixture _fixture; + + public FastDatabaseTests(DatabaseFixture fixture) + { + _fixture = fixture; + } + + [Fact] + public async Task Test1() + { + // Use _fixture.Connection + } + + [Fact] + public async Task Test2() + { + // Reuses the same container + } +} + +// Shared fixture +public class DatabaseFixture : IAsyncLifetime +{ + private readonly TestcontainersContainer _container; + public IDbConnection Connection { get; private set; } + + public DatabaseFixture() + { + _container = new TestcontainersBuilder() + .WithImage("mcr.microsoft.com/mssql/server:2022-latest") + .WithEnvironment("ACCEPT_EULA", "Y") + .WithEnvironment("SA_PASSWORD", "Your_password123") + .WithPortBinding(1433, true) + .Build(); + } + + public async Task InitializeAsync() + { + await _container.StartAsync(); + // Setup connection + } + + public async Task DisposeAsync() + { + await Connection.DisposeAsync(); + await _container.DisposeAsync(); + } +} + +[CollectionDefinition("Database collection")] +public class DatabaseCollection : ICollectionFixture { } +``` + +## Database Reset with Respawn + +When reusing containers, use [Respawn](https://github.com/jbogard/Respawn) to reset database state between tests: + +```xml + +``` + +### Basic Respawn Setup + +```csharp +using Respawn; + +public class DatabaseFixture : IAsyncLifetime +{ + private readonly TestcontainersContainer _container; + private Respawner _respawner = null!; + public NpgsqlConnection Connection { get; private set; } = null!; + public string ConnectionString { get; private set; } = null!; + + public async Task InitializeAsync() + { + await _container.StartAsync(); + + var port = _container.GetMappedPublicPort(5432); + ConnectionString = $"Host=localhost;Port={port};Database=testdb;Username=postgres;Password=postgres"; + + Connection = new NpgsqlConnection(ConnectionString); + await Connection.OpenAsync(); + + await RunMigrationsAsync(); + + _respawner = await Respawner.CreateAsync(ConnectionString, new RespawnerOptions + { + TablesToIgnore = new Table[] + { + "__EFMigrationsHistory", + "AspNetRoles", + "schema_version" + }, + DbAdapter = DbAdapter.Postgres + }); + } + + public async Task ResetDatabaseAsync() + { + await _respawner.ResetAsync(ConnectionString); + } + + public async Task DisposeAsync() + { + await Connection.DisposeAsync(); + await _container.DisposeAsync(); + } +} +``` + +### Using Respawn in Tests + +```csharp +[Collection("Database collection")] +public class OrderTests : IAsyncLifetime +{ + private readonly DatabaseFixture _fixture; + + public OrderTests(DatabaseFixture fixture) + { + _fixture = fixture; + } + + public async Task InitializeAsync() + { + await _fixture.ResetDatabaseAsync(); + } + + public Task DisposeAsync() => Task.CompletedTask; + + [Fact] + public async Task CreateOrder_ShouldPersist() + { + await _fixture.Connection.ExecuteAsync( + "INSERT INTO orders (customer_id, total) VALUES (@CustomerId, @Total)", + new { CustomerId = "CUST1", Total = 100.00m }); + + var count = await _fixture.Connection.QuerySingleAsync( + "SELECT COUNT(*) FROM orders"); + + Assert.Equal(1, count); + } + + [Fact] + public async Task AnotherTest_StartsWithCleanDatabase() + { + var count = await _fixture.Connection.QuerySingleAsync( + "SELECT COUNT(*) FROM orders"); + + Assert.Equal(0, count); // Clean slate! + } +} +``` + +### Respawn Options + +```csharp +var respawner = await Respawner.CreateAsync(connectionString, new RespawnerOptions +{ + TablesToIgnore = new Table[] + { + "__EFMigrationsHistory", + new Table("public", "lookup_data"), + }, + SchemasToInclude = new[] { "public", "app" }, + SchemasToExclude = new[] { "audit", "logging" }, + DbAdapter = DbAdapter.Postgres, + WithReseed = true +}); +``` + +### Why Respawn Over Container Recreation + +| Approach | Pros | Cons | +|----------|------|------| +| **New container per test** | Complete isolation | Slow (10-30s per container) | +| **Respawn** | Fast (~50ms), preserves schema/migrations | Requires careful table exclusion | +| **Transaction rollback** | Fastest | Can't test commit behavior | From 797f2a1c08e56673701f5ef736f266e1ac5eeb8a Mon Sep 17 00:00:00 2001 From: Wouter Van Ranst Date: Sun, 12 Apr 2026 14:35:53 +0200 Subject: [PATCH 2/8] feat: add more skills/agents from https://github.com/Aaronontheweb/dotnet-skills --- .agents/dotnet-benchmark-designer.md | 87 +++++++++++++ .agents/dotnet-concurrency-specialist.md | 81 +++++++++++++ .agents/dotnet-performance-analyst.md | 114 ++++++++++++++++++ .../skills/crap-analysis/SKILL.md | 0 .../skills/csharp-api-design/SKILL.md | 0 .../skills/csharp-coding-standards/SKILL.md | 0 .../anti-patterns-and-reflection.md | 0 .../composition-and-error-handling.md | 0 .../performance-and-api-design.md | 0 .../value-objects-and-patterns.md | 0 .../csharp-type-design-performance/SKILL.md | 0 .../skills/ilspy-decompile/SKILL.md | 0 .../SKILL.md | 0 .../advanced-patterns.md | 0 .../skills/package-management/SKILL.md | 0 .../skills/project-structure/SKILL.md | 0 .../skills/serialization/SKILL.md | 0 .../skills/slopwatch/SKILL.md | 0 .../skills/testcontainers/SKILL.md | 0 .../testcontainers/database-patterns.md | 0 .../testcontainers/infrastructure-patterns.md | 0 21 files changed, 282 insertions(+) create mode 100644 .agents/dotnet-benchmark-designer.md create mode 100644 .agents/dotnet-concurrency-specialist.md create mode 100644 .agents/dotnet-performance-analyst.md rename {.github => .agents}/skills/crap-analysis/SKILL.md (100%) rename {.github => .agents}/skills/csharp-api-design/SKILL.md (100%) rename {.github => .agents}/skills/csharp-coding-standards/SKILL.md (100%) rename {.github => .agents}/skills/csharp-coding-standards/anti-patterns-and-reflection.md (100%) rename {.github => .agents}/skills/csharp-coding-standards/composition-and-error-handling.md (100%) rename {.github => .agents}/skills/csharp-coding-standards/performance-and-api-design.md (100%) rename {.github => .agents}/skills/csharp-coding-standards/value-objects-and-patterns.md (100%) rename {.github => .agents}/skills/csharp-type-design-performance/SKILL.md (100%) rename {.github => .agents}/skills/ilspy-decompile/SKILL.md (100%) rename {.github => .agents}/skills/microsoft-extensions-configuration/SKILL.md (100%) rename {.github => .agents}/skills/microsoft-extensions-configuration/advanced-patterns.md (100%) rename {.github => .agents}/skills/package-management/SKILL.md (100%) rename {.github => .agents}/skills/project-structure/SKILL.md (100%) rename {.github => .agents}/skills/serialization/SKILL.md (100%) rename {.github => .agents}/skills/slopwatch/SKILL.md (100%) rename {.github => .agents}/skills/testcontainers/SKILL.md (100%) rename {.github => .agents}/skills/testcontainers/database-patterns.md (100%) rename {.github => .agents}/skills/testcontainers/infrastructure-patterns.md (100%) diff --git a/.agents/dotnet-benchmark-designer.md b/.agents/dotnet-benchmark-designer.md new file mode 100644 index 00000000..3d9b5595 --- /dev/null +++ b/.agents/dotnet-benchmark-designer.md @@ -0,0 +1,87 @@ +--- +name: dotnet-benchmark-designer +description: Expert in designing effective .NET performance benchmarks and instrumentation. Specializes in BenchmarkDotNet patterns, custom benchmark design, profiling setup, and choosing the right measurement approach for different scenarios. Knows when BenchmarkDotNet isn't suitable and custom benchmarks are needed. +--- + +You are a .NET performance benchmark design specialist with expertise in creating accurate, reliable, and meaningful performance tests. + +**Core Expertise Areas:** + +**BenchmarkDotNet Mastery:** +- Benchmark attribute patterns and configuration +- Job configuration for different runtime targets +- Memory diagnostics and allocation measurement +- Statistical analysis configuration and interpretation +- Parameterized benchmarks and data sources +- Setup/cleanup lifecycle management +- Export formats and CI integration + +**When BenchmarkDotNet Isn't Suitable:** +- Large-scale integration scenarios requiring complex setup +- Long-running benchmarks (>30 seconds) with state transitions +- Multi-process or distributed system measurements +- Real-time performance monitoring during production load +- Benchmarks requiring external system coordination +- Memory-mapped files or system resource interaction + +**Custom Benchmark Design:** +- Stopwatch vs QueryPerformanceCounter usage +- GC measurement and pressure analysis +- Thread contention and CPU utilization metrics +- Custom metric collection and aggregation +- Baseline establishment and storage strategies +- Statistical significance and confidence intervals + +**Profiling Integration:** +- JetBrains dotTrace integration for CPU profiling +- JetBrains dotMemory for memory allocation analysis +- ETW (Event Tracing for Windows) custom events +- PerfView and custom ETW providers +- Continuous profiling in benchmark scenarios + +**Instrumentation Patterns:** +- Activity and DiagnosticSource integration +- Performance counter creation and monitoring +- Custom metrics collection without affecting performance +- Async operation measurement challenges +- Lock-free measurement techniques + +**Benchmark Categories:** +- **Micro-benchmarks**: Single method/operation measurement +- **Component benchmarks**: Class or module-level testing +- **Integration benchmarks**: Multi-component interaction +- **Load benchmarks**: Sustained performance under load +- **Regression benchmarks**: Change impact measurement + +**Design Principles:** +- Minimize measurement overhead and observer effect +- Establish proper warmup and iteration counts +- Control for environmental variables (GC, JIT, CPU affinity) +- Design for repeatability and determinism +- Plan for baseline storage and comparison +- Consider statistical power and sample sizes + +**Common Anti-Patterns to Avoid:** +- Measuring in Debug mode or with debugger attached +- Insufficient warmup causing JIT compilation noise +- Shared state between benchmark iterations +- Console output or logging during measurement +- Synchronous blocking in async benchmarks +- Ignoring GC impact on allocation-heavy operations +- [Benchmark(Baseline = true)] on multiple benchmarks - use categories instead + +**Benchmark Code Generation:** +When creating benchmarks, generate complete, runnable code including: +- Proper using statements and namespace organization +- BenchmarkDotNet attributes and configuration +- Setup and cleanup methods +- Parameter sources and data initialization +- Memory diagnostic configuration when relevant +- Export configuration for results analysis + +**Measurement Strategy Selection:** +Help choose between: +- BenchmarkDotNet for isolated, repeatable micro/component tests +- Custom harnesses for integration or long-running scenarios +- Profiler-assisted measurement for bottleneck identification +- Production monitoring for real-world performance validation diff --git a/.agents/dotnet-concurrency-specialist.md b/.agents/dotnet-concurrency-specialist.md new file mode 100644 index 00000000..2e06e906 --- /dev/null +++ b/.agents/dotnet-concurrency-specialist.md @@ -0,0 +1,81 @@ +--- +name: dotnet-concurrency-specialist +description: Expert in .NET concurrency, threading, and race condition analysis. Specializes in Task/async patterns, thread safety, synchronization primitives, and identifying timing-dependent bugs in multithreaded .NET applications. Use for analyzing racy unit tests, deadlocks, and concurrent code issues. +--- + +You are a .NET concurrency specialist with deep expertise in multithreading, async programming, and race condition diagnosis. + +**Core Expertise Areas:** + +**.NET Threading Fundamentals:** +- Thread vs ThreadPool vs Task execution models +- Thread safety and memory model guarantees +- Volatile fields, memory barriers, and CPU caching effects +- ThreadLocal storage and thread-specific state +- Thread lifecycle and disposal patterns + +**Async/Await and Task Patterns:** +- Task creation, scheduling, and completion +- ConfigureAwait(false) implications and context switching +- Task synchronization and coordination patterns +- Deadlock scenarios with sync-over-async +- TaskCompletionSource and manual task control +- Cancellation tokens and cooperative cancellation + +**Synchronization Primitives:** +- Lock statements and Monitor class behavior +- Mutex, Semaphore, and SemaphoreSlim usage +- ReaderWriterLock patterns and upgrade scenarios +- ManualResetEvent and AutoResetEvent coordination +- Barrier and CountdownEvent for multi-phase operations +- Interlocked operations for lock-free programming + +**Race Condition Patterns:** +- Read-modify-write races and compound operations +- Check-then-act patterns and TOCTOU issues +- Lazy initialization races and double-checked locking +- Collection modification during enumeration +- Resource disposal races and object lifecycle +- Static initialization and type constructor races + +**Common .NET Race Scenarios:** +- Dictionary/ConcurrentDictionary usage patterns +- Event handler registration/deregistration races +- Timer callback overlapping and disposal +- IDisposable implementation races +- Finalizer thread interactions +- Assembly loading and type initialization races + +**Testing and Debugging:** +- Identifying non-deterministic test failures +- Stress testing techniques for race conditions +- Memory model considerations in test scenarios +- Using Thread.Sleep vs proper synchronization in tests +- Debugging tools: Concurrency Visualizer, PerfView +- Static analysis for thread safety issues + +**Diagnostic Approach:** +When analyzing race conditions: +1. Identify shared state and access patterns +2. Map thread boundaries and execution contexts +3. Analyze synchronization mechanisms in use +4. Look for timing assumptions and order dependencies +5. Check for proper resource cleanup and disposal +6. Evaluate async boundaries and context marshaling + +**Anti-Patterns to Identify:** +- Synchronous blocking on async operations +- Improper lock ordering leading to deadlocks +- Missing synchronization on shared mutable state +- Assuming method call atomicity without proper locking +- Race-prone lazy initialization patterns +- Incorrect use of volatile for complex operations +- Thread.Sleep() for coordination instead of proper signaling + +**Race Condition Root Causes:** +- CPU instruction reordering and compiler optimizations +- Cache coherency delays between CPU cores +- Thread scheduling quantum and preemption points +- Garbage collection thread suspension effects +- Just-in-time compilation timing variations +- Hardware-specific timing differences diff --git a/.agents/dotnet-performance-analyst.md b/.agents/dotnet-performance-analyst.md new file mode 100644 index 00000000..00eb472c --- /dev/null +++ b/.agents/dotnet-performance-analyst.md @@ -0,0 +1,114 @@ +--- +name: dotnet-performance-analyst +description: Expert in analyzing .NET application performance data, profiling results, and benchmark comparisons. Specializes in JetBrains profiler analysis, BenchmarkDotNet result interpretation, baseline comparisons, regression detection, and performance bottleneck identification. +--- + +You are a .NET performance analysis specialist with expertise in interpreting profiling data, benchmark results, and identifying performance bottlenecks. + +**Core Expertise Areas:** + +**JetBrains Profiler Analysis:** +- **dotTrace CPU profiling**: Call tree analysis, hot path identification, thread contention +- **dotMemory analysis**: Memory allocation patterns, GC pressure, memory leaks +- Timeline profiling interpretation and UI responsiveness analysis +- Performance counter correlation with profiler data +- Sampling vs tracing profiler mode selection and interpretation + +**BenchmarkDotNet Results Analysis:** +- Statistical interpretation: mean, median, standard deviation significance +- Percentile analysis and outlier identification +- Memory allocation analysis and GC impact assessment +- Scaling analysis across different input sizes +- Cross-platform performance comparison +- CI/CD performance regression detection + +**Baseline Management and Comparison:** +- Establishing performance baselines from historical data +- Regression detection algorithms and thresholds +- Performance trend analysis over time +- Environmental factor normalization (hardware, OS, .NET version) +- Statistical significance testing for performance changes +- Performance budget establishment and monitoring + +**Bottleneck Identification Patterns:** +- **CPU-bound**: Hot methods, algorithm complexity, loop optimization +- **Memory-bound**: Allocation patterns, GC pressure, memory layout +- **I/O-bound**: Async operation efficiency, batching opportunities +- **Lock contention**: Synchronization bottlenecks, thread starvation +- **Cache misses**: Data locality and access patterns +- **JIT compilation**: Warmup characteristics and tier compilation + +**Performance Metrics Interpretation:** +- Throughput vs latency trade-offs and optimization targets +- Percentile analysis (P50, P95, P99) for SLA compliance +- Resource utilization correlation (CPU, memory, I/O) +- Garbage collection impact on application performance +- Thread pool starvation and async operation efficiency + +**Data Analysis Techniques:** +- Time series analysis for performance trends +- Statistical process control for regression detection +- Correlation analysis between metrics and environmental factors +- A/B testing interpretation for performance optimizations +- Load testing result analysis and capacity planning + +**Reporting and Recommendations:** +- Performance improvement priority ranking +- Cost-benefit analysis for optimization efforts +- Risk assessment for performance changes +- Actionable optimization recommendations with code examples +- Performance monitoring and alerting strategy design + +**Hot-Path Delegate Allocation Analysis:** +- **Closure allocations**: Lambdas capturing outer variables allocate per invocation + - `context => next.Invoke(context)` captures `next` — allocate once at build time + - `item => Process(item, constant)` is fine; `item => Process(item, state)` allocates +- **Method-group allocations**: Passing method group to delegate parameter allocates + - `behavior.Invoke(ctx, Next)` where `Next` is a method — cache as `Func` field + - Use static generic cache classes: `static class NextCache { public static readonly Func Next = ...; }` +- **Bound vs unbound delegates**: `next.Invoke` (bound) vs `context => next.Invoke(context)` (closure) + - Prefer bound method-group when delegate signature matches exactly +- **Proactive review**: Always audit delegate construction in hot paths before benchmarking + - Look for: lambda expressions, method groups passed as arguments, `new Func<...>`, `Delegate.CreateDelegate` + - Ask: "Does this allocate per call or per pipeline build?" + +**Common Performance Issues to Identify:** +- **Sync-over-async deadlocks** and context switching overhead +- **Boxing/unboxing** in hot paths and generic constraints +- **String concatenation** and StringBuilder usage patterns +- **LINQ performance** in hot paths vs explicit loops +- **Exception handling** overhead in normal flow +- **Reflection usage** and compilation vs interpretation costs +- **Large Object Heap** pressure and compaction issues + +**Profiler Data Correlation:** +- Cross-reference CPU and memory profiler results +- Correlate GC events with performance degradation +- Map thread contention to specific synchronization points +- Identify resource leaks through allocation tracking +- Connect performance issues to specific code paths + +**Regression Analysis Framework:** +- Establish statistical confidence for performance changes +- Account for environmental variability and measurement noise +- Identify performance improvements vs degradations +- Root cause analysis for performance regressions +- Historical trend analysis and seasonality detection + +**Performance Optimization Validation:** +- Before/after comparison methodology +- Multi-metric impact assessment (throughput, latency, memory) +- Unintended consequence identification +- Performance optimization ROI calculation +- Long-term stability assessment of optimizations + +**Dispatch and Call Pattern Predictions:** +- **Be conservative predicting dispatch optimizations**: Virtual calls, delegate invocations, and interface calls have nuanced JIT behavior + - Don't assume delegate-factory beats virtual dispatch without benchmarking + - Devirtualization benefits depend on sealed types, NGEN/R2R, and call site patterns + - Extra indirection layers often cost more than predicted + - Assumptions may change with newer .NET versions +- **Benchmark competing approaches**: When comparing call patterns (virtual vs delegate vs interface), implement both and measure + - Small differences in call overhead can compound in deep pipelines + - Success path behavior may differ from exception path behavior +- **Trust measurements over intuition**: JIT inlining decisions, register allocation, and CPU cache effects are hard to predict diff --git a/.github/skills/crap-analysis/SKILL.md b/.agents/skills/crap-analysis/SKILL.md similarity index 100% rename from .github/skills/crap-analysis/SKILL.md rename to .agents/skills/crap-analysis/SKILL.md diff --git a/.github/skills/csharp-api-design/SKILL.md b/.agents/skills/csharp-api-design/SKILL.md similarity index 100% rename from .github/skills/csharp-api-design/SKILL.md rename to .agents/skills/csharp-api-design/SKILL.md diff --git a/.github/skills/csharp-coding-standards/SKILL.md b/.agents/skills/csharp-coding-standards/SKILL.md similarity index 100% rename from .github/skills/csharp-coding-standards/SKILL.md rename to .agents/skills/csharp-coding-standards/SKILL.md diff --git a/.github/skills/csharp-coding-standards/anti-patterns-and-reflection.md b/.agents/skills/csharp-coding-standards/anti-patterns-and-reflection.md similarity index 100% rename from .github/skills/csharp-coding-standards/anti-patterns-and-reflection.md rename to .agents/skills/csharp-coding-standards/anti-patterns-and-reflection.md diff --git a/.github/skills/csharp-coding-standards/composition-and-error-handling.md b/.agents/skills/csharp-coding-standards/composition-and-error-handling.md similarity index 100% rename from .github/skills/csharp-coding-standards/composition-and-error-handling.md rename to .agents/skills/csharp-coding-standards/composition-and-error-handling.md diff --git a/.github/skills/csharp-coding-standards/performance-and-api-design.md b/.agents/skills/csharp-coding-standards/performance-and-api-design.md similarity index 100% rename from .github/skills/csharp-coding-standards/performance-and-api-design.md rename to .agents/skills/csharp-coding-standards/performance-and-api-design.md diff --git a/.github/skills/csharp-coding-standards/value-objects-and-patterns.md b/.agents/skills/csharp-coding-standards/value-objects-and-patterns.md similarity index 100% rename from .github/skills/csharp-coding-standards/value-objects-and-patterns.md rename to .agents/skills/csharp-coding-standards/value-objects-and-patterns.md diff --git a/.github/skills/csharp-type-design-performance/SKILL.md b/.agents/skills/csharp-type-design-performance/SKILL.md similarity index 100% rename from .github/skills/csharp-type-design-performance/SKILL.md rename to .agents/skills/csharp-type-design-performance/SKILL.md diff --git a/.github/skills/ilspy-decompile/SKILL.md b/.agents/skills/ilspy-decompile/SKILL.md similarity index 100% rename from .github/skills/ilspy-decompile/SKILL.md rename to .agents/skills/ilspy-decompile/SKILL.md diff --git a/.github/skills/microsoft-extensions-configuration/SKILL.md b/.agents/skills/microsoft-extensions-configuration/SKILL.md similarity index 100% rename from .github/skills/microsoft-extensions-configuration/SKILL.md rename to .agents/skills/microsoft-extensions-configuration/SKILL.md diff --git a/.github/skills/microsoft-extensions-configuration/advanced-patterns.md b/.agents/skills/microsoft-extensions-configuration/advanced-patterns.md similarity index 100% rename from .github/skills/microsoft-extensions-configuration/advanced-patterns.md rename to .agents/skills/microsoft-extensions-configuration/advanced-patterns.md diff --git a/.github/skills/package-management/SKILL.md b/.agents/skills/package-management/SKILL.md similarity index 100% rename from .github/skills/package-management/SKILL.md rename to .agents/skills/package-management/SKILL.md diff --git a/.github/skills/project-structure/SKILL.md b/.agents/skills/project-structure/SKILL.md similarity index 100% rename from .github/skills/project-structure/SKILL.md rename to .agents/skills/project-structure/SKILL.md diff --git a/.github/skills/serialization/SKILL.md b/.agents/skills/serialization/SKILL.md similarity index 100% rename from .github/skills/serialization/SKILL.md rename to .agents/skills/serialization/SKILL.md diff --git a/.github/skills/slopwatch/SKILL.md b/.agents/skills/slopwatch/SKILL.md similarity index 100% rename from .github/skills/slopwatch/SKILL.md rename to .agents/skills/slopwatch/SKILL.md diff --git a/.github/skills/testcontainers/SKILL.md b/.agents/skills/testcontainers/SKILL.md similarity index 100% rename from .github/skills/testcontainers/SKILL.md rename to .agents/skills/testcontainers/SKILL.md diff --git a/.github/skills/testcontainers/database-patterns.md b/.agents/skills/testcontainers/database-patterns.md similarity index 100% rename from .github/skills/testcontainers/database-patterns.md rename to .agents/skills/testcontainers/database-patterns.md diff --git a/.github/skills/testcontainers/infrastructure-patterns.md b/.agents/skills/testcontainers/infrastructure-patterns.md similarity index 100% rename from .github/skills/testcontainers/infrastructure-patterns.md rename to .agents/skills/testcontainers/infrastructure-patterns.md From e91fb4e092b198b307a46c8d8a4d90b636502cf6 Mon Sep 17 00:00:00 2001 From: Wouter Van Ranst Date: Sun, 12 Apr 2026 12:29:36 +0200 Subject: [PATCH 3/8] docs: split parallel finalization and prefix strategy proposals --- .../.openspec.yaml | 2 ++ .../chunk-index-prefix-strategy/proposal.md | 34 +++++++++++++++++++ .../proposal.md | 31 +++++++++-------- 3 files changed, 53 insertions(+), 14 deletions(-) create mode 100644 openspec/changes/chunk-index-prefix-strategy/.openspec.yaml create mode 100644 openspec/changes/chunk-index-prefix-strategy/proposal.md diff --git a/openspec/changes/chunk-index-prefix-strategy/.openspec.yaml b/openspec/changes/chunk-index-prefix-strategy/.openspec.yaml new file mode 100644 index 00000000..33d01f78 --- /dev/null +++ b/openspec/changes/chunk-index-prefix-strategy/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-04-12 diff --git a/openspec/changes/chunk-index-prefix-strategy/proposal.md b/openspec/changes/chunk-index-prefix-strategy/proposal.md new file mode 100644 index 00000000..6bcc3dd3 --- /dev/null +++ b/openspec/changes/chunk-index-prefix-strategy/proposal.md @@ -0,0 +1,34 @@ +## Why + +The chunk-index shard prefix is currently hardcoded to 4 hex characters. That may not remain the best tradeoff as repositories scale: a shorter fixed prefix such as 3 hex characters could reduce the number of shard blobs, while a longer fixed prefix or a dynamic prefix policy could limit shard growth and hot-spotting. This question is larger than a simple performance tweak because shard-prefix strategy affects repository layout, cache behavior, and recovery semantics. + +Chunk-index writes are not transactional across blobs. A run can upload some shard blobs and not others, another machine can extend overlapping prefixes concurrently, and local L1/L2 caches can observe mixed old/new state. Any future prefix strategy work therefore needs to treat durability and recoverability as first-class concerns, not just shard-count optimization. + +## What Changes + +- **Generalize prefix strategy**: replace the implicit "always 4 hex chars" assumption with an explicit chunk-index prefix strategy that can later be implemented as either a fixed length (for example always 3 or always 4 chars) or a dynamic prefix-length policy. +- **Explore tradeoffs before implementation**: compare fixed and dynamic strategies across shard count, shard size, lookup cost, flush cost, cache pressure, and operational simplicity. +- **Define durability and recoverability rules**: specify what correctness means under partial flushes, crashes, concurrent writers, cache invalidation, and mixed local/remote state while blob storage remains non-transactional across shard blobs. +- **Decide repository contract**: determine whether the chosen prefix strategy is repository metadata, configuration, a versioned format decision, or a deterministic algorithm derived from repository state. +- **Plan rollout and recovery**: define how caches and remote layout transitions are detected, invalidated, recovered, or migrated before any prefix change ships. +- **Require recovery proof**: add an integration test that deletes or corrupts the local and remote chunk-index state for a repository and proves Arius can rebuild a correct chunk-index view from scratch rather than depending on intact shard state. + +## Non-goals + +- Changing shard prefix length as part of `parallel-flush-and-tree-upload`. +- Choosing a final prefix strategy in this proposal without the durability and recoverability model. + +## Capabilities + +### New Capabilities +- `chunk-index-prefix-strategy`: Explicit repository rules for chunk-index shard partitioning, including future support for fixed or dynamic prefix length strategies. + +### Modified Capabilities +- `blob-storage`: Chunk-index shard layout becomes a governed repository concern rather than an unexamined hardcoded constant. +- `archive-pipeline`: Future flush and lookup behavior may depend on the repository's declared chunk-index prefix strategy. + +## Impact + +- **Storage contract**: future implementation may affect `chunk-index/` naming, cache layout, or repository metadata. +- **Correctness work first**: durability, recoverability, mixed-state handling, and a recovery-from-scratch test are explicit design constraints for the follow-up phase. +- **Separation of concerns**: finalization parallelism can proceed now without blocking on a shard-layout decision. diff --git a/openspec/changes/parallel-flush-and-tree-upload/proposal.md b/openspec/changes/parallel-flush-and-tree-upload/proposal.md index 8c67ccd8..552089e9 100644 --- a/openspec/changes/parallel-flush-and-tree-upload/proposal.md +++ b/openspec/changes/parallel-flush-and-tree-upload/proposal.md @@ -1,28 +1,31 @@ ## Why -The archive pipeline's end-of-pipeline phases — chunk-index flush and filetree build/upload — are entirely sequential today. Chunk-index shards are flushed one at a time with `foreach`, tree blobs are uploaded one at a time via `EnsureUploadedAsync`, and the two phases run back-to-back. At 1M-chunk scale (~29K touched shards, hundreds of tree blobs), this takes over 4 hours. Parallelizing these independent I/O-bound operations and running the two phases concurrently can reduce this to seconds. Additionally, reducing the shard prefix from 4 hex chars to 3 consolidates 65,536 possible shards into 4,096, reducing per-shard overhead at scale. Neither phase emits progress events, leaving the user with no feedback during the longest part of an archive run. +The archive pipeline still ends with a serialized finalization tail. After uploads complete, it validates filetree cache state, flushes chunk-index shards one prefix at a time, sorts the manifest, builds filetrees while uploading each tree immediately, and only then creates the snapshot. The codebase has evolved since this idea was first proposed: `FileTreeBuilder` now owns the `FileTreeService.ValidateAsync` precondition for tree existence checks, and shard-prefix layout is a separate storage-contract question. We want to speed up finalization without coupling it to a repository layout change. ## What Changes -- **Parallel chunk-index flush**: `ChunkIndexService.FlushAsync` changes from sequential `foreach` over modified shards to `Parallel.ForEachAsync` with configurable concurrency (default 32 workers). Each shard's load → merge → serialize → upload pipeline is independent. -- **3-char shard prefix**: **BREAKING** — The chunk-index shard key changes from 4 hex characters (65,536 shards) to 3 hex characters (4,096 shards). Shard blob paths change from `chunk-index/abcd` to `chunk-index/abc`. At 1M entries this yields ~244 entries/shard (~5 KB gzipped) — a good balance between shard count and shard size. This is a development-only breaking change; no migration is needed. -- **Separate tree compute from upload**: Tree construction splits into two phases: (1) sequential hash computation (CPU-bound, ~ms per blob), then (2) parallel upload of all tree blobs that need uploading, using `Parallel.ForEachAsync` with 32 workers. -- **Concurrent flush + tree build**: Chunk-index flush and tree build/upload run concurrently via `Task.WhenAll` instead of sequentially. These phases are independent — flush writes to `chunk-index/`, tree build writes to `filetrees/`. -- **Progress events for flush and tree upload**: Two new notification events: `ChunkIndexFlushProgressEvent(int ShardsCompleted, int TotalShards)` and `TreeUploadProgressEvent(int BlobsUploaded, int TotalBlobs)`. These enable the CLI to show two parallel progress lines during the end-of-pipeline phase. +- **Parallel chunk-index flush**: `ChunkIndexService.FlushAsync` keeps the current shard format but flushes touched prefixes in parallel with configurable concurrency. Each worker performs load -> merge -> serialize -> upload -> cache update for one prefix. +- **Separate tree hash computation from upload**: `FileTreeBuilder.BuildAsync` performs one validation pass, computes all directory tree blobs and hashes from the sorted manifest without further remote calls, then uploads only missing tree blobs in parallel. +- **Concurrent finalization work**: after manifest writing completes, archive finalization overlaps independent work instead of running it strictly back-to-back. Chunk-index flush runs concurrently with manifest sort, tree hash computation, and tree upload where dependencies allow; snapshot creation still waits for both finalization branches to finish. +- **Progress events for finalization**: add `ChunkIndexFlushProgressEvent(int ShardsCompleted, int TotalShards)` and `TreeUploadProgressEvent(int BlobsUploaded, int TotalBlobs)` so the CLI can show explicit progress during the longest end-of-pipeline phase. +- **Single validation owner**: the archive pipeline keeps one owner for filetree validation. If `FileTreeBuilder.BuildAsync` validates before using `ExistsInRemote`, `ArchiveCommandHandler` SHALL not perform a redundant pre-validation call. + +## Non-goals + +- Changing chunk-index shard prefix length or blob naming format. That storage-layout work is tracked separately. ## Capabilities ### New Capabilities -- `parallel-flush-and-tree-upload`: Parallel chunk-index flush, parallel tree blob upload, concurrent execution of both via `Task.WhenAll`, 3-char shard prefix, and progress events for both phases. +- `parallel-flush-and-tree-upload`: Parallel chunk-index flush, parallel tree blob upload, overlapped end-of-pipeline execution, and explicit finalization progress events. ### Modified Capabilities -- `archive-pipeline`: End-of-pipeline changes from sequential flush → build → snapshot to concurrent (flush ∥ tree-build) → snapshot. New progress events emitted during flush and tree upload. -- `blob-storage`: Chunk-index shard blob path format changes from `chunk-index/<4-char-prefix>` to `chunk-index/<3-char-prefix>`. +- `archive-pipeline`: End-of-pipeline changes from serialized validate -> flush -> sort -> build -> snapshot to an overlapped finalization flow with one validation owner and parallel progress reporting. ## Impact -- **Core layer** (`Arius.Core`): `ChunkIndexService.FlushAsync` and `FileTreeBuilder.BuildAsync` change significantly. `ArchiveCommandHandler` wires `Task.WhenAll` for the two phases. -- **Shard path format**: `chunk-index/` blob paths change from 4-char to 3-char prefixes. Existing L2 disk cache files become stale (acceptable — cache miss falls through to L3). Existing Azure shards from prior development archives become orphaned (acceptable — still in development, no production data). -- **New events**: `ChunkIndexFlushProgressEvent` and `TreeUploadProgressEvent` added to archive events. CLI display layer will consume these for parallel progress lines. -- **Performance**: At 1M-chunk scale, estimated end-of-pipeline time drops from ~265 minutes (sequential) to ~13 seconds (parallel, epoch-match fast path) or ~23 seconds (slow path with prefetch). -- **Tests**: New tests for parallel flush (verify all shards uploaded, concurrency correctness), parallel tree upload, concurrent execution, 3-char prefix shard paths, and progress event emission. +- **Core layer** (`Arius.Core`): `ChunkIndexService.FlushAsync` and `FileTreeBuilder.BuildAsync` change significantly. `ArchiveCommandHandler` changes orchestration only where concurrency boundaries belong there. +- **No repository layout change**: `chunk-index/` naming and current shard prefix length stay unchanged in this change. +- **New events**: `ChunkIndexFlushProgressEvent` and `TreeUploadProgressEvent` are added to archive events and consumed by CLI progress display. +- **Performance focus**: this change targets the end-of-pipeline tail dominated by sequential shard flush and sequential tree upload, while leaving shard-layout optimization for a follow-up change. +- **Tests**: add coverage for parallel flush correctness, two-phase tree build/upload behavior, concurrent finalization orchestration, single-owner validation, and progress event emission. From b5e917f022a28c63ab2e623e70914719649b5009 Mon Sep 17 00:00:00 2001 From: Wouter Van Ranst Date: Sun, 12 Apr 2026 14:39:34 +0200 Subject: [PATCH 4/8] docs: flesh out parallel finalization change --- .../parallel-flush-and-tree-upload/design.md | 173 ++++++++++++++++++ .../proposal.md | 8 +- .../parallel-flush-and-tree-upload/tasks.md | 32 ++++ 3 files changed, 211 insertions(+), 2 deletions(-) create mode 100644 openspec/changes/parallel-flush-and-tree-upload/design.md create mode 100644 openspec/changes/parallel-flush-and-tree-upload/tasks.md diff --git a/openspec/changes/parallel-flush-and-tree-upload/design.md b/openspec/changes/parallel-flush-and-tree-upload/design.md new file mode 100644 index 00000000..dec87f2c --- /dev/null +++ b/openspec/changes/parallel-flush-and-tree-upload/design.md @@ -0,0 +1,173 @@ +## Context + +The archive pipeline already uses substantial parallelism for hashing and chunk upload, but the finalization tail is still largely serialized. After upload work completes, `ArchiveCommandHandler` currently does the following in order: + +1. `FileTreeService.ValidateAsync()` +2. `ChunkIndexService.FlushAsync()` +3. `ManifestSorter.SortAsync()` +4. `FileTreeBuilder.BuildAsync()` +5. `SnapshotService.CreateAsync()` + +This shape leaves two distinct sources of end-of-pipeline latency: + +- `ChunkIndexService.FlushAsync()` flushes touched shard prefixes one at a time. +- `FileTreeBuilder.BuildAsync()` computes tree blobs bottom-up and uploads each missing tree immediately, so upload latency is paid inline during tree construction. + +The codebase has evolved since this idea was first proposed. `FileTreeBuilder.BuildAsync()` now calls `FileTreeService.ValidateAsync()` itself before `ExistsInRemote()`, which means the validation precondition already lives inside the tree-building boundary. The old proposal also bundled shard-prefix changes, but shard layout is a separate repository-contract problem and is out of scope here. + +The design also has to respect Arius's scale and durability constraints. Arius is a backup tool for important files. Repositories may be large in total bytes and still contain many thousands of small files, so file-count scale matters as much as byte scale for archive, restore, and list operations. Blob storage is non-transactional across blobs, mutable caches can be stale or corrupt, and partial updates are unavoidable in crash scenarios. Finalization work therefore must stay recoverable, retry-safe, and clearly separated from the repository commit point. + +## Goals / Non-Goals + +**Goals:** +- Reduce archive end-of-pipeline wall-clock time without changing repository layout. +- Keep one clear owner for filetree validation. +- Allow chunk-index flush to use parallel workers across independent shard prefixes. +- Split tree building into a local hash-computation phase and a parallel upload phase without requiring unbounded memory. +- Emit explicit progress events for finalization so the CLI shows useful progress after uploads finish. +- Preserve durability semantics: snapshot creation remains the only repository commit point, and pre-snapshot work must be recoverable after interruption. + +**Non-Goals:** +- Changing chunk-index shard prefix length, shard naming, or repository metadata. +- Solving durability/recoverability for alternative shard-prefix strategies. +- Reworking the manifest sort algorithm beyond fitting it into the new orchestration shape. +- Moving archive orchestration out of `ArchiveCommandHandler` into Shared. + +## Decisions + +### Decision 1: `FileTreeBuilder` remains the validation owner + +`FileTreeBuilder.BuildAsync()` will remain responsible for ensuring `FileTreeService.ValidateAsync()` has run before any call to `ExistsInRemote()`. + +That means `ArchiveCommandHandler` will stop doing a redundant explicit pre-validation call. The builder already owns the precondition for tree existence checks, and keeping validation ownership there avoids splitting one invariant across two layers. + +Alternative considered: keep validation in `ArchiveCommandHandler` and remove it from `FileTreeBuilder`. +Rejected because it makes `FileTreeBuilder` easier to misuse outside the archive handler and weakens the service boundary that has already emerged. + +### Decision 2: Tree building becomes a two-phase operation + +`FileTreeBuilder.BuildAsync()` will be refactored into a bounded producer/consumer pipeline: + +1. Validate once. +2. Read the sorted manifest. +3. Build directory entry sets and compute tree hashes bottom-up without remote I/O. +4. For each computed tree hash, use `ExistsInRemote()` to decide whether upload is needed. +5. For trees that need upload, serialize plaintext bytes to a temporary spool file outside the final cache directory and enqueue a descriptor into a bounded channel. +6. Parallel upload workers consume descriptors, upload the tree blob, then publish the plaintext bytes into the final filetree cache path only after upload succeeds. +7. Return the root tree hash after upload completion. + +The important observation is that parent directory hashes depend on child directory hashes, not on child uploads. Tree upload is therefore a persistence concern, not a computation dependency. + +The temporary spool file is important for both memory pressure and correctness: + +- It avoids holding all computed `FileTreeBlob` instances in memory when the repository has many directories. +- It allows compute and upload to overlap. +- It avoids publishing a final cache file before remote upload is known to have succeeded. Temp spool files are tentative local state; final filetree cache files represent confirmed remote existence. + +Alternative considered: keep the current inline `EnsureUploadedAsync()` call during bottom-up traversal and only parallelize the upload call itself. +Rejected because the current structure still serializes existence checks and remote upload latency into the tree computation loop. + +Alternative considered: compute all trees in memory first, then upload from memory. +Rejected because file-count scale can be large even when tree blobs are individually small; bounded disk-backed spooling is a better fit for Arius's scale assumptions. + +### Decision 3: `ChunkIndexService.FlushAsync()` parallelizes by shard prefix + +Flush workers will operate independently per touched shard prefix: + +`load existing shard -> merge pending entries for prefix -> serialize -> upload -> save L2 -> promote L1` + +The concurrency unit is one prefix. Prefixes are independent because each worker writes a distinct `chunk-index/` blob and a distinct L2 file. + +Shared mutable state inside `ChunkIndexService` is limited to: + +- draining and grouping pending entries before worker launch +- L1 cache promotion under the existing lock +- progress counters + +Alternative considered: overlapping shard upload work while retaining sequential load/merge. +Rejected because most of the per-prefix latency is in the whole end-to-end prefix pipeline, not just the final upload call. + +### Decision 4: Archive finalization overlaps only across real dependency boundaries + +The new orchestration is not "everything parallel with everything." The dependency graph is: + +```text +flush ----------------------------\ + -> snapshot +sort -> compute tree hashes -> upload / +``` + +So the archive handler will overlap: + +- chunk-index flush branch +- manifest sort + tree branch + +`SnapshotService.CreateAsync()` still waits until both branches are complete. + +Alternative considered: keep the archive handler fully sequential and rely only on internal service-level parallelism. +Rejected because manifest sort and tree work are independent of chunk-index flush once manifest writing is complete, so handler-level overlap is real and valuable. + +### Decision 5: Finalization progress is modeled as two explicit event streams + +Add two notification events: + +- `ChunkIndexFlushProgressEvent(int ShardsCompleted, int TotalShards)` +- `TreeUploadProgressEvent(int BlobsUploaded, int TotalBlobs)` + +These are emitted by the components doing the work, not synthesized afterward. The CLI can then render finalization progress as two concurrent progress lines instead of appearing stalled between upload completion and snapshot creation. + +Alternative considered: a single generic "archive phase changed" event. +Rejected because it loses the two independent denominators the CLI needs for meaningful progress display. + +### Decision 6: Manifest sort remains a separate prerequisite for tree computation + +`ManifestSorter.SortAsync()` stays separate for now. We will overlap it with chunk-index flush but not fold it into tree-building changes in this design. + +This keeps the scope on finalization parallelism rather than mixing in a larger manifest-sorting redesign. If sort later becomes a dominant bottleneck, that can be addressed independently. + +There is one manifest temp file per archive run, not multiple manifests sorted in parallel. The relevant concurrency is therefore between the single manifest-sort/tree branch and the chunk-index flush branch. + +### Decision 7: Snapshot remains the only repository commit point + +Parallel finalization work may leave partial remote state behind if the run fails: some chunk-index shards may have flushed and some filetrees may have uploaded. That is acceptable only because the repository is not considered committed until `SnapshotService.CreateAsync()` publishes the new snapshot. + +This design relies on the following invariant: + +- Pre-snapshot finalization artifacts are recoverable tentative state. +- The latest snapshot is the only authoritative description of repository state. +- Local caches must never convert tentative local work into claimed remote truth prematurely. + +That is why tree upload temp spool files are separate from the final filetree cache path, and why cache validation remains explicit. + +### Decision 8: Terminology note for manifest vs snapshot + +The current domain language uses `manifest` for two different things: + +- the temporary on-disk file-entry input to tree building +- the durable snapshot JSON object stored under `snapshots/` + +That overlap is confusing. This change does not rename either artifact, but it records the issue so domain language can later be normalized. A likely direction is to reserve `snapshot` for the durable repository commit object and use a more specific term such as `tree-build manifest` or `archive manifest` for the temporary sorted-input file. + +## Risks / Trade-offs + +- **Parallel flush increases service-internal concurrency pressure** -> Mitigation: snapshot pending entries into per-prefix groups before worker launch, keep L1 promotion under the existing lock, and use one progress counter per flush operation. +- **Tree compute/upload pipeline can increase temp-disk usage** -> Mitigation: use bounded channels, delete temp spool files after upload, and keep spool files outside the final cache path so crash recovery can distinguish tentative local work from confirmed remote state. +- **Slow-path validation may still dominate some runs** -> Mitigation: accept that the slow path is a different cost center. This change focuses on the serialized flush and tree-upload tail, while validation remains correct and explicit. +- **Progress events from parallel workers can arrive out of order** -> Mitigation: events carry completed/total counts rather than assuming ordered delivery. +- **Concurrency split across handler and services can blur ownership** -> Mitigation: keep validation and tree-upload internals inside `FileTreeBuilder`, keep per-prefix shard work inside `ChunkIndexService`, and let `ArchiveCommandHandler` only compose the two high-level branches. +- **Partial remote updates can look like corruption** -> Mitigation: treat snapshot publication as the commit point, design caches as hints rather than truth, and keep finalization idempotent and retry-safe. + +## Migration Plan + +1. Remove the redundant archive-handler call to `FileTreeService.ValidateAsync()`. +2. Refactor `FileTreeBuilder.BuildAsync()` into local compute first, then parallel upload of missing tree blobs. +3. Refactor `ChunkIndexService.FlushAsync()` to snapshot touched prefixes and flush them in parallel. +4. Update `ArchiveCommandHandler` to overlap flush with sort/tree finalization while preserving snapshot-after-both semantics. +5. Add finalization progress events and CLI handlers. +6. Add tests for validation ownership, bounded disk-spooled tree behavior, parallel flush correctness, orchestration ordering, and progress emission. + +Rollback is straightforward because the change does not alter repository layout: restore sequential orchestration and the prior inline tree upload behavior if the new concurrency shape proves awkward. + +## Open Questions + +- Whether `ManifestSorter.SortAsync()` is small enough in practice to leave as-is once flush and tree upload are parallelized. diff --git a/openspec/changes/parallel-flush-and-tree-upload/proposal.md b/openspec/changes/parallel-flush-and-tree-upload/proposal.md index 552089e9..cb535901 100644 --- a/openspec/changes/parallel-flush-and-tree-upload/proposal.md +++ b/openspec/changes/parallel-flush-and-tree-upload/proposal.md @@ -2,13 +2,16 @@ The archive pipeline still ends with a serialized finalization tail. After uploads complete, it validates filetree cache state, flushes chunk-index shards one prefix at a time, sorts the manifest, builds filetrees while uploading each tree immediately, and only then creates the snapshot. The codebase has evolved since this idea was first proposed: `FileTreeBuilder` now owns the `FileTreeService.ValidateAsync` precondition for tree existence checks, and shard-prefix layout is a separate storage-contract question. We want to speed up finalization without coupling it to a repository layout change. +This change must remain grounded in Arius's operating constraints. Arius is a backup tool for important files, so durability and recoverability matter more than shaving off a little complexity. Repository scale can be large in bytes and large in file count: potentially terabyte-scale binary content with many thousands of small files. Blob storage is non-transactional across blobs, and local cache state can be stale or corrupt. Any finalization parallelism must preserve a clear commit point at snapshot creation and remain recoverable after partial uploads, partial shard flushes, and cache divergence. + ## What Changes - **Parallel chunk-index flush**: `ChunkIndexService.FlushAsync` keeps the current shard format but flushes touched prefixes in parallel with configurable concurrency. Each worker performs load -> merge -> serialize -> upload -> cache update for one prefix. -- **Separate tree hash computation from upload**: `FileTreeBuilder.BuildAsync` performs one validation pass, computes all directory tree blobs and hashes from the sorted manifest without further remote calls, then uploads only missing tree blobs in parallel. +- **Bounded tree compute/upload pipeline**: `FileTreeBuilder.BuildAsync` performs one validation pass, computes directory tree blobs and hashes from the sorted manifest without further remote calls, writes upload-needed trees to bounded temporary spool files on disk, and lets parallel upload workers consume that spool. This preserves low memory usage even when the repository contains many directories. - **Concurrent finalization work**: after manifest writing completes, archive finalization overlaps independent work instead of running it strictly back-to-back. Chunk-index flush runs concurrently with manifest sort, tree hash computation, and tree upload where dependencies allow; snapshot creation still waits for both finalization branches to finish. - **Progress events for finalization**: add `ChunkIndexFlushProgressEvent(int ShardsCompleted, int TotalShards)` and `TreeUploadProgressEvent(int BlobsUploaded, int TotalBlobs)` so the CLI can show explicit progress during the longest end-of-pipeline phase. - **Single validation owner**: the archive pipeline keeps one owner for filetree validation. If `FileTreeBuilder.BuildAsync` validates before using `ExistsInRemote`, `ArchiveCommandHandler` SHALL not perform a redundant pre-validation call. +- **Terminology cleanup note**: this change records that `manifest` currently refers to the temporary file-entry input to tree building, while `snapshot manifest` refers to the durable repository commit object. The naming overlap is confusing and should be normalized in domain language/docs as a follow-up consistency cleanup. ## Non-goals @@ -28,4 +31,5 @@ The archive pipeline still ends with a serialized finalization tail. After uploa - **No repository layout change**: `chunk-index/` naming and current shard prefix length stay unchanged in this change. - **New events**: `ChunkIndexFlushProgressEvent` and `TreeUploadProgressEvent` are added to archive events and consumed by CLI progress display. - **Performance focus**: this change targets the end-of-pipeline tail dominated by sequential shard flush and sequential tree upload, while leaving shard-layout optimization for a follow-up change. -- **Tests**: add coverage for parallel flush correctness, two-phase tree build/upload behavior, concurrent finalization orchestration, single-owner validation, and progress event emission. +- **Durability constraints**: partial finalization work before snapshot creation is acceptable only as recoverable orphaned state; snapshot creation remains the repository commit point. +- **Tests**: add coverage for parallel flush correctness, bounded tree compute/upload behavior, concurrent finalization orchestration, single-owner validation, and progress event emission. diff --git a/openspec/changes/parallel-flush-and-tree-upload/tasks.md b/openspec/changes/parallel-flush-and-tree-upload/tasks.md new file mode 100644 index 00000000..c5e1338f --- /dev/null +++ b/openspec/changes/parallel-flush-and-tree-upload/tasks.md @@ -0,0 +1,32 @@ +## 1. Validation Ownership And Finalization Shape + +- [ ] 1.1 Remove the redundant `ArchiveCommandHandler` call to `FileTreeService.ValidateAsync()` so `FileTreeBuilder` remains the single validation owner for tree existence checks +- [ ] 1.2 Refactor archive end-of-pipeline orchestration so chunk-index flush overlaps with the manifest-sort/tree branch while `SnapshotService.CreateAsync()` still waits for both branches to finish +- [ ] 1.3 Add or update tests that assert snapshot creation does not occur until both flush and tree upload work complete + +## 2. Parallel Chunk-Index Flush + +- [ ] 2.1 Refactor `ChunkIndexService.FlushAsync()` to snapshot pending entries into per-prefix groups before worker launch +- [ ] 2.2 Flush touched shard prefixes in parallel with bounded concurrency, preserving the existing per-prefix load -> merge -> serialize -> upload -> L2 save -> L1 promote flow +- [ ] 2.3 Emit `ChunkIndexFlushProgressEvent(int ShardsCompleted, int TotalShards)` during parallel flush +- [ ] 2.4 Add tests for parallel flush correctness, including all touched prefixes uploaded, L2/L1 cache updates preserved, and duplicate/overlapping content hashes merged correctly under concurrency + +## 3. Bounded Tree Compute/Upload Pipeline + +- [ ] 3.1 Refactor `FileTreeBuilder.BuildAsync()` so directory hash computation remains local after validation and does not depend on remote upload completion +- [ ] 3.2 Introduce a bounded producer/consumer pipeline for upload-needed tree blobs: compute writes temporary spool files outside the final filetree cache directory, upload workers consume descriptors from a bounded channel +- [ ] 3.3 Publish plaintext tree bytes into the final filetree cache path only after upload succeeds, so crash recovery can distinguish tentative local spool state from confirmed remote existence +- [ ] 3.4 Emit `TreeUploadProgressEvent(int BlobsUploaded, int TotalBlobs)` during tree upload +- [ ] 3.5 Add tests for bounded disk-spooled tree upload behavior, including deduplicated trees skipped without upload, successful uploads populating the final cache, and interrupted runs not leaving false-positive final cache entries + +## 4. CLI Finalization Progress + +- [ ] 4.1 Add CLI notification handlers and progress-state support for chunk-index flush progress and tree upload progress +- [ ] 4.2 Update archive progress rendering so finalization shows explicit concurrent progress instead of appearing stalled after chunk upload completes +- [ ] 4.3 Add CLI tests that verify finalization progress remains correct when progress events arrive out of order from parallel workers + +## 5. Verification + +- [ ] 5.1 Run unit and integration tests covering archive finalization, filetree caching, chunk-index flush, and archive CLI progress +- [ ] 5.2 Add a regression test documenting that snapshot creation remains the only repository commit point: partial flush/tree-upload work before snapshot must not be treated as a completed archive state +- [ ] 5.3 Review domain-language/docs touchpoints for the temporary archive manifest versus durable snapshot naming overlap and capture any follow-up cleanup work without renaming storage artifacts in this change From 1fd77d40e5196a1512b44da860d9dc1591330872 Mon Sep 17 00:00:00 2001 From: Wouter Van Ranst Date: Sun, 12 Apr 2026 15:14:21 +0200 Subject: [PATCH 5/8] feat: parallelize archive finalization --- AGENTS.md | 10 +- README.md | 14 + .../parallel-flush-and-tree-upload/tasks.md | 36 +-- .../Archive/BuildArchiveDisplayTests.cs | 38 +++ .../Archive/NotificationHandlerTests.cs | 42 +++ .../MediatorEventRoutingIntegrationTests.cs | 6 + .../Archive/ArchiveProgressHandlers.cs | 22 ++ src/Arius.Cli/Commands/Archive/ArchiveVerb.cs | 15 + src/Arius.Cli/ProgressState.cs | 26 ++ .../ArchiveFinalizationTests.cs | 167 ++++++++++++ .../CoordinatedArchiveBlobContainerService.cs | 132 +++++++++ .../ChunkIndex/ChunkIndexServiceTests.cs | 133 +++++++++ ...RecordingChunkIndexBlobContainerService.cs | 150 ++++++++++ .../RecordingFileTreeBlobContainerService.cs | 157 +++++++++++ .../Shared/FileTree/FileTreeBuilderTests.cs | 113 ++++++++ .../ArchiveCommand/ArchiveCommandHandler.cs | 33 ++- .../Features/ArchiveCommand/Events.cs | 8 +- .../Shared/ChunkIndex/ChunkIndexService.cs | 80 +++--- .../Shared/FileTree/FileTreeBuilder.cs | 257 ++++++++++++------ 19 files changed, 1287 insertions(+), 152 deletions(-) create mode 100644 src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveFinalizationTests.cs create mode 100644 src/Arius.Core.Tests/Features/ArchiveCommand/Fakes/CoordinatedArchiveBlobContainerService.cs create mode 100644 src/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexServiceTests.cs create mode 100644 src/Arius.Core.Tests/Shared/ChunkIndex/Fakes/RecordingChunkIndexBlobContainerService.cs create mode 100644 src/Arius.Core.Tests/Shared/FileTree/Fakes/RecordingFileTreeBlobContainerService.cs diff --git a/AGENTS.md b/AGENTS.md index aaa8ef54..ee6f9cd5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -13,6 +13,13 @@ - Always update `README.md` (for humans) and `AGENTS.md` (for AI coding agents) to reflect the current state of the project +## Durability And Scale + +- Arius is a backup/archive tool. Prefer recoverability and correctness over throughput. +- Snapshot creation is the only repository commit point. Chunk-index flushes and filetree uploads that complete before a snapshot exists are tentative partial state, not a completed archive. +- Parallelize independent work when useful, but do not weaken crash recovery semantics to do it. +- When local cache publication depends on remote durability, prefer a spool-then-publish flow: write tentative data outside the final cache path, upload it, then publish the final cache entry only after upload succeeds. + ## Testing This project uses **TUnit** (not xUnit/NUnit). Key differences: @@ -48,8 +55,9 @@ This project uses **TUnit** (not xUnit/NUnit). Key differences: - **thin chunk**: a small pointer-like chunk blob whose body is the hash of the tar chunk that actually contains the file bytes. Why: as deduplication existence check and metadata. - **chunk index**: the repository-wide mapping from content hash to chunk hash. Why: 1/ TAR lookups 2/ efficient existence checks for deduplicated content and 3/ metadata store. - **shard**: one mutable chunk-index blob, partitioned by hash prefix for storage and caching. +- **archive manifest**: the temporary per-run file-entry list written during archive, then externally sorted and consumed by `FileTreeBuilder`. It is not a durable repository commit object. - **filetree**: an immutable Merkle-tree blob describing one directory's entries. Filetrees model repository structure, not chunk storage. -- **snapshot**: an immutable point-in-time manifest that records the root filetree hash and repository totals. +- **snapshot**: an immutable point-in-time manifest that records the root filetree hash and repository totals. Snapshot creation is the repository commit point. - Prefer these terms consistently in code, tests, docs, and reviews. Avoid using generic words like "blob" or "pointer" when the more precise domain term is known. diff --git a/README.md b/README.md index 686442d2..d096ab4d 100644 --- a/README.md +++ b/README.md @@ -101,6 +101,20 @@ dotnet user-secrets set "arius::key" "" Most test projects can be run directly with `dotnet test --project `. `src/Arius.E2E.Tests` also requires `ARIUS_E2E_ACCOUNT` and `ARIUS_E2E_KEY` to be set; otherwise the suite fails immediately with a configuration error. +## Archive Finalization + +Archive writes a temporary `archive manifest` on disk, externally sorts it, builds/uploads +filetrees, flushes chunk-index shards, and only then creates the snapshot. + +Chunk-index flush and filetree upload now run in parallel during finalization, but snapshot +creation is still the only repository commit point. If a run crashes after some shard or +filetree uploads but before the snapshot is written, that work is treated as incomplete +state rather than a completed archive. + +To keep the domain language precise, use `archive manifest` for the temporary tree-build +input file and `snapshot` or `snapshot manifest` for the durable repository state stored +under `snapshots/`. + ## Updating Run: diff --git a/openspec/changes/parallel-flush-and-tree-upload/tasks.md b/openspec/changes/parallel-flush-and-tree-upload/tasks.md index c5e1338f..3e7b4f11 100644 --- a/openspec/changes/parallel-flush-and-tree-upload/tasks.md +++ b/openspec/changes/parallel-flush-and-tree-upload/tasks.md @@ -1,32 +1,32 @@ ## 1. Validation Ownership And Finalization Shape -- [ ] 1.1 Remove the redundant `ArchiveCommandHandler` call to `FileTreeService.ValidateAsync()` so `FileTreeBuilder` remains the single validation owner for tree existence checks -- [ ] 1.2 Refactor archive end-of-pipeline orchestration so chunk-index flush overlaps with the manifest-sort/tree branch while `SnapshotService.CreateAsync()` still waits for both branches to finish -- [ ] 1.3 Add or update tests that assert snapshot creation does not occur until both flush and tree upload work complete +- [x] 1.1 Remove the redundant `ArchiveCommandHandler` call to `FileTreeService.ValidateAsync()` so `FileTreeBuilder` remains the single validation owner for tree existence checks +- [x] 1.2 Refactor archive end-of-pipeline orchestration so chunk-index flush overlaps with the manifest-sort/tree branch while `SnapshotService.CreateAsync()` still waits for both branches to finish +- [x] 1.3 Add or update tests that assert snapshot creation does not occur until both flush and tree upload work complete ## 2. Parallel Chunk-Index Flush -- [ ] 2.1 Refactor `ChunkIndexService.FlushAsync()` to snapshot pending entries into per-prefix groups before worker launch -- [ ] 2.2 Flush touched shard prefixes in parallel with bounded concurrency, preserving the existing per-prefix load -> merge -> serialize -> upload -> L2 save -> L1 promote flow -- [ ] 2.3 Emit `ChunkIndexFlushProgressEvent(int ShardsCompleted, int TotalShards)` during parallel flush -- [ ] 2.4 Add tests for parallel flush correctness, including all touched prefixes uploaded, L2/L1 cache updates preserved, and duplicate/overlapping content hashes merged correctly under concurrency +- [x] 2.1 Refactor `ChunkIndexService.FlushAsync()` to snapshot pending entries into per-prefix groups before worker launch +- [x] 2.2 Flush touched shard prefixes in parallel with bounded concurrency, preserving the existing per-prefix load -> merge -> serialize -> upload -> L2 save -> L1 promote flow +- [x] 2.3 Emit `ChunkIndexFlushProgressEvent(int ShardsCompleted, int TotalShards)` during parallel flush +- [x] 2.4 Add tests for parallel flush correctness, including all touched prefixes uploaded, L2/L1 cache updates preserved, and duplicate/overlapping content hashes merged correctly under concurrency ## 3. Bounded Tree Compute/Upload Pipeline -- [ ] 3.1 Refactor `FileTreeBuilder.BuildAsync()` so directory hash computation remains local after validation and does not depend on remote upload completion -- [ ] 3.2 Introduce a bounded producer/consumer pipeline for upload-needed tree blobs: compute writes temporary spool files outside the final filetree cache directory, upload workers consume descriptors from a bounded channel -- [ ] 3.3 Publish plaintext tree bytes into the final filetree cache path only after upload succeeds, so crash recovery can distinguish tentative local spool state from confirmed remote existence -- [ ] 3.4 Emit `TreeUploadProgressEvent(int BlobsUploaded, int TotalBlobs)` during tree upload -- [ ] 3.5 Add tests for bounded disk-spooled tree upload behavior, including deduplicated trees skipped without upload, successful uploads populating the final cache, and interrupted runs not leaving false-positive final cache entries +- [x] 3.1 Refactor `FileTreeBuilder.BuildAsync()` so directory hash computation remains local after validation and does not depend on remote upload completion +- [x] 3.2 Introduce a bounded producer/consumer pipeline for upload-needed tree blobs: compute writes temporary spool files outside the final filetree cache directory, upload workers consume descriptors from a bounded channel +- [x] 3.3 Publish plaintext tree bytes into the final filetree cache path only after upload succeeds, so crash recovery can distinguish tentative local spool state from confirmed remote existence +- [x] 3.4 Emit `TreeUploadProgressEvent(int BlobsUploaded, int TotalBlobs)` during tree upload +- [x] 3.5 Add tests for bounded disk-spooled tree upload behavior, including deduplicated trees skipped without upload, successful uploads populating the final cache, and interrupted runs not leaving false-positive final cache entries ## 4. CLI Finalization Progress -- [ ] 4.1 Add CLI notification handlers and progress-state support for chunk-index flush progress and tree upload progress -- [ ] 4.2 Update archive progress rendering so finalization shows explicit concurrent progress instead of appearing stalled after chunk upload completes -- [ ] 4.3 Add CLI tests that verify finalization progress remains correct when progress events arrive out of order from parallel workers +- [x] 4.1 Add CLI notification handlers and progress-state support for chunk-index flush progress and tree upload progress +- [x] 4.2 Update archive progress rendering so finalization shows explicit concurrent progress instead of appearing stalled after chunk upload completes +- [x] 4.3 Add CLI tests that verify finalization progress remains correct when progress events arrive out of order from parallel workers ## 5. Verification -- [ ] 5.1 Run unit and integration tests covering archive finalization, filetree caching, chunk-index flush, and archive CLI progress -- [ ] 5.2 Add a regression test documenting that snapshot creation remains the only repository commit point: partial flush/tree-upload work before snapshot must not be treated as a completed archive state -- [ ] 5.3 Review domain-language/docs touchpoints for the temporary archive manifest versus durable snapshot naming overlap and capture any follow-up cleanup work without renaming storage artifacts in this change +- [x] 5.1 Run unit and integration tests covering archive finalization, filetree caching, chunk-index flush, and archive CLI progress +- [x] 5.2 Add a regression test documenting that snapshot creation remains the only repository commit point: partial flush/tree-upload work before snapshot must not be treated as a completed archive state +- [x] 5.3 Review domain-language/docs touchpoints for the temporary archive manifest versus durable snapshot naming overlap and capture any follow-up cleanup work without renaming storage artifacts in this change diff --git a/src/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayTests.cs b/src/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayTests.cs index b37ff82d..c70a8558 100644 --- a/src/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayTests.cs +++ b/src/Arius.Cli.Tests/Commands/Archive/BuildArchiveDisplayTests.cs @@ -155,6 +155,44 @@ public void BuildArchiveDisplay_UploadingHeader_OpenCircle_WhenNotComplete() uploadingLine.ShouldContain("○"); } + [Test] + public void BuildArchiveDisplay_ShowsChunkIndexFinalizationProgress_WhenPresent() + { + var state = new ProgressState(); + state.SetChunkIndexFlushProgress(2, 5); + + var output = RenderToString(ArchiveVerb.BuildDisplay(state)); + var line = GetLineContaining(output, "Finalizing Index"); + + line.ShouldContain("2 / 5 shards"); + line.ShouldContain("○"); + } + + [Test] + public void BuildArchiveDisplay_ShowsTreeUploadProgress_WhenPresent() + { + var state = new ProgressState(); + state.SetTreeUploadProgress(3, 4); + + var output = RenderToString(ArchiveVerb.BuildDisplay(state)); + var line = GetLineContaining(output, "Uploading Trees"); + + line.ShouldContain("3 / 4 blobs"); + line.ShouldContain("○"); + } + + [Test] + public void BuildArchiveDisplay_FinalizationRows_UseFilledCircle_WhenComplete() + { + var state = new ProgressState(); + state.SetChunkIndexFlushProgress(5, 5); + state.SetTreeUploadProgress(4, 4); + + var output = RenderToString(ArchiveVerb.BuildDisplay(state)); + GetLineContaining(output, "Finalizing Index").ShouldContain("●"); + GetLineContaining(output, "Uploading Trees").ShouldContain("●"); + } + // ── 6.4 Per-file lines: only Hashing or Uploading ──────────────────────── [Test] diff --git a/src/Arius.Cli.Tests/Commands/Archive/NotificationHandlerTests.cs b/src/Arius.Cli.Tests/Commands/Archive/NotificationHandlerTests.cs index 7f632bfe..e988e043 100644 --- a/src/Arius.Cli.Tests/Commands/Archive/NotificationHandlerTests.cs +++ b/src/Arius.Cli.Tests/Commands/Archive/NotificationHandlerTests.cs @@ -245,4 +245,46 @@ public async Task SnapshotCreatedHandler_SetsSnapshotComplete() state.SnapshotComplete.ShouldBeTrue(); } + + [Test] + public async Task ChunkIndexFlushProgressHandler_SetsFinalizationProgress() + { + var state = new ProgressState(); + var handler = new ChunkIndexFlushProgressHandler(state); + + await handler.Handle(new ChunkIndexFlushProgressEvent(2, 5), CancellationToken.None); + + state.ChunkIndexFlushCompletedShards.ShouldBe(2); + state.ChunkIndexFlushTotalShards.ShouldBe(5); + } + + [Test] + public async Task TreeUploadProgressHandler_SetsFinalizationProgress() + { + var state = new ProgressState(); + var handler = new TreeUploadProgressHandler(state); + + await handler.Handle(new TreeUploadProgressEvent(4, 7), CancellationToken.None); + + state.TreeUploadCompletedBlobs.ShouldBe(4); + state.TreeUploadTotalBlobs.ShouldBe(7); + } + + [Test] + public async Task FinalizationProgressHandlers_KeepLatestCounts_WhenEventsArriveOutOfOrder() + { + var state = new ProgressState(); + var flushHandler = new ChunkIndexFlushProgressHandler(state); + var treeHandler = new TreeUploadProgressHandler(state); + + await flushHandler.Handle(new ChunkIndexFlushProgressEvent(3, 5), CancellationToken.None); + await flushHandler.Handle(new ChunkIndexFlushProgressEvent(2, 5), CancellationToken.None); + await treeHandler.Handle(new TreeUploadProgressEvent(4, 7), CancellationToken.None); + await treeHandler.Handle(new TreeUploadProgressEvent(1, 7), CancellationToken.None); + + state.ChunkIndexFlushCompletedShards.ShouldBe(2); + state.ChunkIndexFlushTotalShards.ShouldBe(5); + state.TreeUploadCompletedBlobs.ShouldBe(1); + state.TreeUploadTotalBlobs.ShouldBe(7); + } } diff --git a/src/Arius.Cli.Tests/MediatorEventRoutingIntegrationTests.cs b/src/Arius.Cli.Tests/MediatorEventRoutingIntegrationTests.cs index a70558f2..9e0f600b 100644 --- a/src/Arius.Cli.Tests/MediatorEventRoutingIntegrationTests.cs +++ b/src/Arius.Cli.Tests/MediatorEventRoutingIntegrationTests.cs @@ -54,6 +54,8 @@ public async Task AllArchiveEvents_UpdateProgressState_ViaMediator() await mediator.Publish(new TarBundleSealingEvent(1, 100, "tar1", ["hash1"])); await mediator.Publish(new ChunkUploadingEvent("tar1", 100)); await mediator.Publish(new TarBundleUploadedEvent("tar1", 80, 1)); + await mediator.Publish(new ChunkIndexFlushProgressEvent(2, 4)); + await mediator.Publish(new TreeUploadProgressEvent(3, 5)); await mediator.Publish(new SnapshotCreatedEvent("root", DateTimeOffset.UtcNow, 1)); // Verify ProgressState was updated @@ -62,6 +64,10 @@ public async Task AllArchiveEvents_UpdateProgressState_ViaMediator() state.ScanComplete.ShouldBeTrue(); state.FilesHashed.ShouldBe(1L); state.TarsUploaded.ShouldBe(1L); + state.ChunkIndexFlushCompletedShards.ShouldBe(2); + state.ChunkIndexFlushTotalShards.ShouldBe(4); + state.TreeUploadCompletedBlobs.ShouldBe(3); + state.TreeUploadTotalBlobs.ShouldBe(5); // a.bin was removed after tar entry added state.TrackedFiles.ContainsKey("a.bin").ShouldBeFalse(); state.SnapshotComplete.ShouldBeTrue(); diff --git a/src/Arius.Cli/Commands/Archive/ArchiveProgressHandlers.cs b/src/Arius.Cli/Commands/Archive/ArchiveProgressHandlers.cs index 8b3319c9..5fda88ef 100644 --- a/src/Arius.Cli/Commands/Archive/ArchiveProgressHandlers.cs +++ b/src/Arius.Cli/Commands/Archive/ArchiveProgressHandlers.cs @@ -185,6 +185,28 @@ public ValueTask Handle(TarBundleUploadedEvent notification, CancellationToken c } } +// ── ChunkIndexFlushProgressHandler ──────────────────────────────────────────── + +public sealed class ChunkIndexFlushProgressHandler(ProgressState state) : INotificationHandler +{ + public ValueTask Handle(ChunkIndexFlushProgressEvent notification, CancellationToken cancellationToken) + { + state.SetChunkIndexFlushProgress(notification.ShardsCompleted, notification.TotalShards); + return ValueTask.CompletedTask; + } +} + +// ── TreeUploadProgressHandler ───────────────────────────────────────────────── + +public sealed class TreeUploadProgressHandler(ProgressState state) : INotificationHandler +{ + public ValueTask Handle(TreeUploadProgressEvent notification, CancellationToken cancellationToken) + { + state.SetTreeUploadProgress(notification.BlobsUploaded, notification.TotalBlobs); + return ValueTask.CompletedTask; + } +} + // ── SnapshotCreatedHandler ──────────────────────────────────────────────────── /// Sets when the snapshot is created. diff --git a/src/Arius.Cli/Commands/Archive/ArchiveVerb.cs b/src/Arius.Cli/Commands/Archive/ArchiveVerb.cs index a04c08ff..9cc12afa 100644 --- a/src/Arius.Cli/Commands/Archive/ArchiveVerb.cs +++ b/src/Arius.Cli/Commands/Archive/ArchiveVerb.cs @@ -270,6 +270,21 @@ internal static IRenderable BuildDisplay(ProgressState state) } } + // ── Finalization headers ────────────────────────────────────────────── + if (state.ChunkIndexFlushTotalShards is { } flushTotal) + { + var flushDone = state.ChunkIndexFlushCompletedShards >= flushTotal; + var flushSymbol = flushDone ? "[green]●[/]" : "[yellow]○[/]"; + lines.Add(new Markup($" {flushSymbol} Finalizing Index [dim]{state.ChunkIndexFlushCompletedShards:N0} / {flushTotal:N0} shards[/]")); + } + + if (state.TreeUploadTotalBlobs is { } treeTotal) + { + var treeDone = state.TreeUploadCompletedBlobs >= treeTotal; + var treeSymbol = treeDone ? "[green]●[/]" : "[yellow]○[/]"; + lines.Add(new Markup($" {treeSymbol} Uploading Trees [dim]{state.TreeUploadCompletedBlobs:N0} / {treeTotal:N0} blobs[/]")); + } + // ── Per-file and TAR bundle rows ────────────────────────────────────── var activeFiles = state.TrackedFiles.Values .Where(f => f.State is FileState.Hashing or FileState.Uploading) diff --git a/src/Arius.Cli/ProgressState.cs b/src/Arius.Cli/ProgressState.cs index 1c32cc26..ad15193c 100644 --- a/src/Arius.Cli/ProgressState.cs +++ b/src/Arius.Cli/ProgressState.cs @@ -434,6 +434,32 @@ public void IncrementChunksUploaded(long compressedSize) /// Marks the snapshot as complete. public void SetSnapshotComplete() => Volatile.Write(ref _snapshotComplete, true); + // ── Archive: finalization progress ─────────────────────────────────────── + + public int? ChunkIndexFlushTotalShards => Volatile.Read(ref _chunkIndexFlushTotalShards) < 0 ? null : Volatile.Read(ref _chunkIndexFlushTotalShards); + private int _chunkIndexFlushTotalShards = -1; + + public int ChunkIndexFlushCompletedShards => Volatile.Read(ref _chunkIndexFlushCompletedShards); + private int _chunkIndexFlushCompletedShards; + + public void SetChunkIndexFlushProgress(int completed, int total) + { + Volatile.Write(ref _chunkIndexFlushCompletedShards, completed); + Volatile.Write(ref _chunkIndexFlushTotalShards, total); + } + + public int? TreeUploadTotalBlobs => Volatile.Read(ref _treeUploadTotalBlobs) < 0 ? null : Volatile.Read(ref _treeUploadTotalBlobs); + private int _treeUploadTotalBlobs = -1; + + public int TreeUploadCompletedBlobs => Volatile.Read(ref _treeUploadCompletedBlobs); + private int _treeUploadCompletedBlobs; + + public void SetTreeUploadProgress(int completed, int total) + { + Volatile.Write(ref _treeUploadCompletedBlobs, completed); + Volatile.Write(ref _treeUploadTotalBlobs, total); + } + // ── Restore ─────────────────────────────────────────────────────────────── // ── Restore: active downloads ──────────────────────────────────────────── diff --git a/src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveFinalizationTests.cs b/src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveFinalizationTests.cs new file mode 100644 index 00000000..46a5ba00 --- /dev/null +++ b/src/Arius.Core.Tests/Features/ArchiveCommand/ArchiveFinalizationTests.cs @@ -0,0 +1,167 @@ +using Arius.Core.Features.ArchiveCommand; +using Arius.Core.Shared; +using Arius.Core.Shared.ChunkIndex; +using Arius.Core.Shared.ChunkStorage; +using Arius.Core.Shared.Encryption; +using Arius.Core.Shared.FileTree; +using Arius.Core.Shared.Snapshot; +using Arius.Core.Shared.Storage; +using Arius.Core.Tests.Features.ArchiveCommand.Fakes; +using Mediator; +using Microsoft.Extensions.Logging.Testing; +using NSubstitute; +using TUnit.Core; +using ArchiveCommandType = Arius.Core.Features.ArchiveCommand.ArchiveCommand; + +namespace Arius.Core.Tests.Features.ArchiveCommand; + +public class ArchiveFinalizationTests +{ + [Test] + public async Task Archive_Finalization_OverlapsFlushWithTreeWork_AndWaitsForFlushBeforeSnapshot() + { + const string account = "acct-finalization"; + var container = $"ctr-finalization-{Guid.NewGuid():N}"; + var root = Path.Combine(Path.GetTempPath(), $"arius-finalization-{Guid.NewGuid():N}"); + Directory.CreateDirectory(root); + + try + { + await File.WriteAllBytesAsync(Path.Combine(root, "large.bin"), new byte[2 * 1024 * 1024]); + await File.WriteAllBytesAsync(Path.Combine(root, "small.txt"), new byte[256]); + + var blobs = new CoordinatedArchiveBlobContainerService(); + var encryption = new PlaintextPassthroughService(); + var index = new ChunkIndexService(blobs, encryption, account, container); + var fileTreeService = new FileTreeService(blobs, encryption, index, account, container); + var snapshotService = new SnapshotService(blobs, encryption, account, container); + var chunkStorage = new ChunkStorageService(blobs, encryption); + var mediator = Substitute.For(); + var logger = new FakeLogger(); + + var handler = new ArchiveCommandHandler( + blobs, + encryption, + index, + chunkStorage, + fileTreeService, + snapshotService, + mediator, + logger, + account, + container); + + var archiveTask = handler.Handle( + new ArchiveCommandType(new ArchiveCommandOptions + { + RootDirectory = root, + UploadTier = BlobTier.Hot, + }), + CancellationToken.None).AsTask(); + + await blobs.TreeUploadStarted.WaitAsync(TimeSpan.FromSeconds(5)); + blobs.IndexUploadCompleted.IsCompleted.ShouldBeFalse("tree upload should start before index flush completes"); + + blobs.AllowTreeUpload(); + blobs.AllowIndexUpload(); + + var result = await archiveTask; + + result.Success.ShouldBeTrue(result.ErrorMessage); + blobs.SnapshotUploadedBeforeIndexCompleted.ShouldBeFalse("snapshot must wait for chunk-index flush completion"); + blobs.IndexUploadCompleted.IsCompleted.ShouldBeTrue(); + + await mediator.Received().Publish( + Arg.Is(e => e.ShardsCompleted >= 1 && e.TotalShards >= 1), + Arg.Any()); + + await mediator.Received().Publish( + Arg.Is(e => e.BlobsUploaded >= 1 && e.TotalBlobs >= 1), + Arg.Any()); + } + finally + { + if (Directory.Exists(root)) + Directory.Delete(root, recursive: true); + + var repoDir = RepositoryPaths.GetRepositoryDirectory(account, container); + if (Directory.Exists(repoDir)) + Directory.Delete(repoDir, recursive: true); + } + } + + [Test] + public async Task Archive_PartialFinalizationBeforeSnapshot_DoesNotCreateCompletedArchiveState() + { + const string account = "acct-commit-point"; + var container = $"ctr-commit-point-{Guid.NewGuid():N}"; + var root = Path.Combine(Path.GetTempPath(), $"arius-commit-point-{Guid.NewGuid():N}"); + Directory.CreateDirectory(root); + + try + { + await File.WriteAllBytesAsync(Path.Combine(root, "large.bin"), new byte[2 * 1024 * 1024]); + await File.WriteAllBytesAsync(Path.Combine(root, "small.txt"), new byte[256]); + + var blobs = new CoordinatedArchiveBlobContainerService(); + var encryption = new PlaintextPassthroughService(); + var index = new ChunkIndexService(blobs, encryption, account, container); + var fileTreeService = new FileTreeService(blobs, encryption, index, account, container); + var snapshotService = new SnapshotService(blobs, encryption, account, container); + var chunkStorage = new ChunkStorageService(blobs, encryption); + var mediator = Substitute.For(); + var logger = new FakeLogger(); + + var handler = new ArchiveCommandHandler( + blobs, + encryption, + index, + chunkStorage, + fileTreeService, + snapshotService, + mediator, + logger, + account, + container); + + var archiveTask = handler.Handle( + new ArchiveCommandType(new ArchiveCommandOptions + { + RootDirectory = root, + UploadTier = BlobTier.Hot, + }), + CancellationToken.None).AsTask(); + + await blobs.TreeUploadStarted.WaitAsync(TimeSpan.FromSeconds(5)); + + blobs.HasAnyBlobWithPrefix(BlobPaths.Snapshots).ShouldBeFalse(); + + blobs.AllowTreeUpload(); + await blobs.TreeUploadCompleted.WaitAsync(TimeSpan.FromSeconds(5)); + + blobs.HasAnyBlobWithPrefix(BlobPaths.FileTrees).ShouldBeTrue(); + blobs.HasAnyBlobWithPrefix(BlobPaths.Snapshots).ShouldBeFalse(); + + var visibleSnapshotBeforeCommit = await snapshotService.ResolveAsync(); + visibleSnapshotBeforeCommit.ShouldBeNull(); + + blobs.AllowIndexUpload(); + + var result = await archiveTask; + + result.Success.ShouldBeTrue(result.ErrorMessage); + + var visibleSnapshotAfterCommit = await snapshotService.ResolveAsync(); + visibleSnapshotAfterCommit.ShouldNotBeNull(); + } + finally + { + if (Directory.Exists(root)) + Directory.Delete(root, recursive: true); + + var repoDir = RepositoryPaths.GetRepositoryDirectory(account, container); + if (Directory.Exists(repoDir)) + Directory.Delete(repoDir, recursive: true); + } + } +} diff --git a/src/Arius.Core.Tests/Features/ArchiveCommand/Fakes/CoordinatedArchiveBlobContainerService.cs b/src/Arius.Core.Tests/Features/ArchiveCommand/Fakes/CoordinatedArchiveBlobContainerService.cs new file mode 100644 index 00000000..ad80a975 --- /dev/null +++ b/src/Arius.Core.Tests/Features/ArchiveCommand/Fakes/CoordinatedArchiveBlobContainerService.cs @@ -0,0 +1,132 @@ +using System.Collections.Concurrent; +using Arius.Core.Shared.Storage; + +namespace Arius.Core.Tests.Features.ArchiveCommand.Fakes; + +internal sealed class CoordinatedArchiveBlobContainerService : IBlobContainerService +{ + private readonly ConcurrentDictionary _blobs = new(StringComparer.Ordinal); + private readonly TaskCompletionSource _treeUploadStarted = new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly TaskCompletionSource _allowTreeUpload = new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly TaskCompletionSource _treeUploadCompleted = new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly TaskCompletionSource _allowIndexUpload = new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly TaskCompletionSource _indexUploadCompleted = new(TaskCreationOptions.RunContinuationsAsynchronously); + + public Task TreeUploadStarted => _treeUploadStarted.Task; + public Task TreeUploadCompleted => _treeUploadCompleted.Task; + public Task IndexUploadCompleted => _indexUploadCompleted.Task; + + public bool SnapshotUploadedBeforeIndexCompleted { get; private set; } + + public void AllowTreeUpload() => _allowTreeUpload.TrySetResult(); + + public void AllowIndexUpload() => _allowIndexUpload.TrySetResult(); + + public bool HasAnyBlobWithPrefix(string prefix) => _blobs.Keys.Any(name => name.StartsWith(prefix, StringComparison.Ordinal)); + + public Task CreateContainerIfNotExistsAsync(CancellationToken cancellationToken = default) => Task.CompletedTask; + + public async Task UploadAsync(string blobName, Stream content, IReadOnlyDictionary metadata, BlobTier tier, string? contentType = null, bool overwrite = false, CancellationToken cancellationToken = default) + { + if (blobName.StartsWith(BlobPaths.FileTrees, StringComparison.Ordinal)) + { + _treeUploadStarted.TrySetResult(); + await _allowTreeUpload.Task.WaitAsync(cancellationToken); + } + + if (blobName.StartsWith(BlobPaths.ChunkIndex, StringComparison.Ordinal)) + await _allowIndexUpload.Task.WaitAsync(cancellationToken); + + if (blobName.StartsWith(BlobPaths.Snapshots, StringComparison.Ordinal) && !_indexUploadCompleted.Task.IsCompleted) + SnapshotUploadedBeforeIndexCompleted = true; + + await using var ms = new MemoryStream(); + await content.CopyToAsync(ms, cancellationToken); + _blobs[blobName] = new StoredBlob(ms.ToArray(), new Dictionary(metadata), tier, contentType, false); + + if (blobName.StartsWith(BlobPaths.FileTrees, StringComparison.Ordinal)) + _treeUploadCompleted.TrySetResult(); + + if (blobName.StartsWith(BlobPaths.ChunkIndex, StringComparison.Ordinal)) + _indexUploadCompleted.TrySetResult(); + } + + public Task OpenWriteAsync(string blobName, string? contentType = null, CancellationToken cancellationToken = default) + => Task.FromResult(new CommitOnDisposeStream(bytes => + { + _blobs[blobName] = new StoredBlob(bytes, new Dictionary(), null, contentType, false); + })); + + public Task DownloadAsync(string blobName, CancellationToken cancellationToken = default) + => Task.FromResult(new MemoryStream(_blobs[blobName].Content, writable: false)); + + public Task GetMetadataAsync(string blobName, CancellationToken cancellationToken = default) + { + if (!_blobs.TryGetValue(blobName, out var blob)) + return Task.FromResult(new BlobMetadata { Exists = false }); + + return Task.FromResult(new BlobMetadata + { + Exists = true, + Tier = blob.Tier, + ContentLength = blob.Content.LongLength, + IsRehydrating = blob.IsRehydrating, + Metadata = new Dictionary(blob.Metadata) + }); + } + + public async IAsyncEnumerable ListAsync(string prefix, [System.Runtime.CompilerServices.EnumeratorCancellation] CancellationToken cancellationToken = default) + { + foreach (var blobName in _blobs.Keys.Where(name => name.StartsWith(prefix, StringComparison.Ordinal)).OrderBy(name => name, StringComparer.Ordinal)) + { + cancellationToken.ThrowIfCancellationRequested(); + yield return blobName; + await Task.Yield(); + } + } + + public Task SetMetadataAsync(string blobName, IReadOnlyDictionary metadata, CancellationToken cancellationToken = default) + { + var blob = _blobs[blobName]; + _blobs[blobName] = blob with { Metadata = new Dictionary(metadata) }; + return Task.CompletedTask; + } + + public Task SetTierAsync(string blobName, BlobTier tier, CancellationToken cancellationToken = default) + { + var blob = _blobs[blobName]; + _blobs[blobName] = blob with { Tier = tier }; + return Task.CompletedTask; + } + + public Task CopyAsync(string sourceBlobName, string destinationBlobName, BlobTier destinationTier, RehydratePriority? rehydratePriority = null, CancellationToken cancellationToken = default) + { + var source = _blobs[sourceBlobName]; + _blobs[destinationBlobName] = source with { Tier = destinationTier, IsRehydrating = false }; + return Task.CompletedTask; + } + + public Task DeleteAsync(string blobName, CancellationToken cancellationToken = default) + { + _blobs.TryRemove(blobName, out _); + return Task.CompletedTask; + } + + private sealed record StoredBlob( + byte[] Content, + Dictionary Metadata, + BlobTier? Tier, + string? ContentType, + bool IsRehydrating); + + private sealed class CommitOnDisposeStream(Action onCommit) : MemoryStream + { + protected override void Dispose(bool disposing) + { + if (disposing) + onCommit(ToArray()); + + base.Dispose(disposing); + } + } +} diff --git a/src/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexServiceTests.cs b/src/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexServiceTests.cs new file mode 100644 index 00000000..9a54b802 --- /dev/null +++ b/src/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexServiceTests.cs @@ -0,0 +1,133 @@ +using Arius.Core.Shared; +using Arius.Core.Shared.ChunkIndex; +using Arius.Core.Shared.Encryption; +using Arius.Core.Tests.Shared.ChunkIndex.Fakes; +using TUnit.Core; + +namespace Arius.Core.Tests.Shared.ChunkIndex; + +public class ChunkIndexServiceTests +{ + private static readonly PlaintextPassthroughService s_encryption = new(); + + [Test] + public async Task FlushAsync_MultiplePrefixes_UsesParallelUploads() + { + const string account = "acct-ci-parallel"; + var container = $"ctr-ci-parallel-{Guid.NewGuid():N}"; + CleanupRepo(account, container); + + try + { + var blobs = new RecordingChunkIndexBlobContainerService(TimeSpan.FromMilliseconds(150)); + var service = new ChunkIndexService(blobs, s_encryption, account, container); + + service.AddEntry(new ShardEntry("aaaa" + new string('1', 60), "chunk-a", 10, 5)); + service.AddEntry(new ShardEntry("bbbb" + new string('2', 60), "chunk-b", 20, 8)); + service.AddEntry(new ShardEntry("cccc" + new string('3', 60), "chunk-c", 30, 12)); + + await service.FlushAsync(); + + blobs.MaxConcurrentChunkIndexUploads.ShouldBeGreaterThan(1); + } + finally + { + CleanupRepo(account, container); + } + } + + [Test] + public async Task FlushAsync_UpdatesL1AndL2Caches_AndMergesExistingShardContent() + { + const string account = "acct-ci-cache"; + var container = $"ctr-ci-cache-{Guid.NewGuid():N}"; + CleanupRepo(account, container); + + try + { + var blobs = new RecordingChunkIndexBlobContainerService(); + var seedService = new ChunkIndexService(blobs, s_encryption, account, container); + var existingEntry = new ShardEntry("aaaa" + new string('0', 60), "chunk-existing", 100, 50); + seedService.AddEntry(existingEntry); + await seedService.FlushAsync(); + + CleanupRepo(account, container); + + var service = new ChunkIndexService(blobs, s_encryption, account, container); + var newEntry = new ShardEntry("aaaa" + new string('1', 60), "chunk-new", 200, 75); + service.AddEntry(newEntry); + + await service.FlushAsync(); + + var metadataReadsBeforeL1 = blobs.ChunkIndexMetadataReads; + var downloadsBeforeL1 = blobs.ChunkIndexDownloads; + + var mergedLookup = await service.LookupAsync(existingEntry.ContentHash); + + mergedLookup.ShouldNotBeNull(); + mergedLookup.ChunkHash.ShouldBe(existingEntry.ChunkHash); + blobs.ChunkIndexMetadataReads.ShouldBe(metadataReadsBeforeL1); + blobs.ChunkIndexDownloads.ShouldBe(downloadsBeforeL1); + + var l2Path = Path.Combine(RepositoryPaths.GetChunkIndexCacheDirectory(account, container), Shard.PrefixOf(existingEntry.ContentHash)); + File.Exists(l2Path).ShouldBeTrue(); + + var metadataReadsBeforeL2 = blobs.ChunkIndexMetadataReads; + var downloadsBeforeL2 = blobs.ChunkIndexDownloads; + var newService = new ChunkIndexService(blobs, s_encryption, account, container); + + var l2Lookup = await newService.LookupAsync(newEntry.ContentHash); + + l2Lookup.ShouldNotBeNull(); + l2Lookup.ChunkHash.ShouldBe(newEntry.ChunkHash); + blobs.ChunkIndexMetadataReads.ShouldBe(metadataReadsBeforeL2); + blobs.ChunkIndexDownloads.ShouldBe(downloadsBeforeL2); + } + finally + { + CleanupRepo(account, container); + } + } + + [Test] + public async Task FlushAsync_ReportsProgress_PerCompletedPrefix() + { + const string account = "acct-ci-progress"; + var container = $"ctr-ci-progress-{Guid.NewGuid():N}"; + CleanupRepo(account, container); + + try + { + var blobs = new RecordingChunkIndexBlobContainerService(); + var service = new ChunkIndexService(blobs, s_encryption, account, container); + var updates = new List<(int Completed, int Total)>(); + var progress = new SynchronousProgress<(int Completed, int Total)>(update => updates.Add(update)); + + service.AddEntry(new ShardEntry("aaaa" + new string('1', 60), "chunk-a", 10, 5)); + service.AddEntry(new ShardEntry("bbbb" + new string('2', 60), "chunk-b", 20, 8)); + service.AddEntry(new ShardEntry("cccc" + new string('3', 60), "chunk-c", 30, 12)); + + await service.FlushAsync(progress); + + updates.Count.ShouldBe(3); + updates.Select(u => u.Total).Distinct().ShouldBe([3]); + updates.Select(u => u.Completed).OrderBy(x => x).ShouldBe([1, 2, 3]); + } + finally + { + CleanupRepo(account, container); + } + } + + private static void CleanupRepo(string account, string container) + { + var repoDir = RepositoryPaths.GetRepositoryDirectory(account, container); + if (Directory.Exists(repoDir)) + Directory.Delete(repoDir, recursive: true); + } + + private sealed class SynchronousProgress(Action onReport) : IProgress + { + public void Report(T value) => onReport(value); + } +} diff --git a/src/Arius.Core.Tests/Shared/ChunkIndex/Fakes/RecordingChunkIndexBlobContainerService.cs b/src/Arius.Core.Tests/Shared/ChunkIndex/Fakes/RecordingChunkIndexBlobContainerService.cs new file mode 100644 index 00000000..50a9f6de --- /dev/null +++ b/src/Arius.Core.Tests/Shared/ChunkIndex/Fakes/RecordingChunkIndexBlobContainerService.cs @@ -0,0 +1,150 @@ +using System.Collections.Concurrent; +using Arius.Core.Shared.Storage; + +namespace Arius.Core.Tests.Shared.ChunkIndex.Fakes; + +internal sealed class RecordingChunkIndexBlobContainerService : IBlobContainerService +{ + private readonly ConcurrentDictionary _blobs = new(StringComparer.Ordinal); + private readonly TimeSpan _chunkIndexUploadDelay; + + private int _activeChunkIndexUploads; + private int _maxConcurrentChunkIndexUploads; + private int _chunkIndexMetadataReads; + private int _chunkIndexDownloads; + + public RecordingChunkIndexBlobContainerService(TimeSpan? chunkIndexUploadDelay = null) + { + _chunkIndexUploadDelay = chunkIndexUploadDelay ?? TimeSpan.Zero; + } + + public int MaxConcurrentChunkIndexUploads => Volatile.Read(ref _maxConcurrentChunkIndexUploads); + public int ChunkIndexMetadataReads => Volatile.Read(ref _chunkIndexMetadataReads); + public int ChunkIndexDownloads => Volatile.Read(ref _chunkIndexDownloads); + + public Task CreateContainerIfNotExistsAsync(CancellationToken cancellationToken = default) => Task.CompletedTask; + + public async Task UploadAsync(string blobName, Stream content, IReadOnlyDictionary metadata, BlobTier tier, string? contentType = null, bool overwrite = false, CancellationToken cancellationToken = default) + { + var isChunkIndex = blobName.StartsWith(BlobPaths.ChunkIndex, StringComparison.Ordinal); + if (isChunkIndex) + { + var active = Interlocked.Increment(ref _activeChunkIndexUploads); + UpdateMaxConcurrency(active); + if (_chunkIndexUploadDelay > TimeSpan.Zero) + await Task.Delay(_chunkIndexUploadDelay, cancellationToken); + } + + try + { + await using var ms = new MemoryStream(); + await content.CopyToAsync(ms, cancellationToken); + _blobs[blobName] = new StoredBlob(ms.ToArray(), new Dictionary(metadata), tier, contentType, false); + } + finally + { + if (isChunkIndex) + Interlocked.Decrement(ref _activeChunkIndexUploads); + } + } + + public Task OpenWriteAsync(string blobName, string? contentType = null, CancellationToken cancellationToken = default) + => Task.FromResult(new CommitOnDisposeStream(bytes => + { + _blobs[blobName] = new StoredBlob(bytes, new Dictionary(), null, contentType, false); + })); + + public Task DownloadAsync(string blobName, CancellationToken cancellationToken = default) + { + if (blobName.StartsWith(BlobPaths.ChunkIndex, StringComparison.Ordinal)) + Interlocked.Increment(ref _chunkIndexDownloads); + + return Task.FromResult(new MemoryStream(_blobs[blobName].Content, writable: false)); + } + + public Task GetMetadataAsync(string blobName, CancellationToken cancellationToken = default) + { + if (blobName.StartsWith(BlobPaths.ChunkIndex, StringComparison.Ordinal)) + Interlocked.Increment(ref _chunkIndexMetadataReads); + + if (!_blobs.TryGetValue(blobName, out var blob)) + return Task.FromResult(new BlobMetadata { Exists = false }); + + return Task.FromResult(new BlobMetadata + { + Exists = true, + Tier = blob.Tier, + ContentLength = blob.Content.LongLength, + IsRehydrating = blob.IsRehydrating, + Metadata = new Dictionary(blob.Metadata) + }); + } + + public async IAsyncEnumerable ListAsync(string prefix, [System.Runtime.CompilerServices.EnumeratorCancellation] CancellationToken cancellationToken = default) + { + foreach (var blobName in _blobs.Keys.Where(name => name.StartsWith(prefix, StringComparison.Ordinal)).OrderBy(name => name, StringComparer.Ordinal)) + { + cancellationToken.ThrowIfCancellationRequested(); + yield return blobName; + await Task.Yield(); + } + } + + public Task SetMetadataAsync(string blobName, IReadOnlyDictionary metadata, CancellationToken cancellationToken = default) + { + var blob = _blobs[blobName]; + _blobs[blobName] = blob with { Metadata = new Dictionary(metadata) }; + return Task.CompletedTask; + } + + public Task SetTierAsync(string blobName, BlobTier tier, CancellationToken cancellationToken = default) + { + var blob = _blobs[blobName]; + _blobs[blobName] = blob with { Tier = tier }; + return Task.CompletedTask; + } + + public Task CopyAsync(string sourceBlobName, string destinationBlobName, BlobTier destinationTier, RehydratePriority? rehydratePriority = null, CancellationToken cancellationToken = default) + { + var source = _blobs[sourceBlobName]; + _blobs[destinationBlobName] = source with { Tier = destinationTier, IsRehydrating = false }; + return Task.CompletedTask; + } + + public Task DeleteAsync(string blobName, CancellationToken cancellationToken = default) + { + _blobs.TryRemove(blobName, out _); + return Task.CompletedTask; + } + + private void UpdateMaxConcurrency(int active) + { + while (true) + { + var current = Volatile.Read(ref _maxConcurrentChunkIndexUploads); + if (active <= current) + return; + + if (Interlocked.CompareExchange(ref _maxConcurrentChunkIndexUploads, active, current) == current) + return; + } + } + + private sealed record StoredBlob( + byte[] Content, + Dictionary Metadata, + BlobTier? Tier, + string? ContentType, + bool IsRehydrating); + + private sealed class CommitOnDisposeStream(Action onCommit) : MemoryStream + { + protected override void Dispose(bool disposing) + { + if (disposing) + onCommit(ToArray()); + + base.Dispose(disposing); + } + } +} diff --git a/src/Arius.Core.Tests/Shared/FileTree/Fakes/RecordingFileTreeBlobContainerService.cs b/src/Arius.Core.Tests/Shared/FileTree/Fakes/RecordingFileTreeBlobContainerService.cs new file mode 100644 index 00000000..72457e60 --- /dev/null +++ b/src/Arius.Core.Tests/Shared/FileTree/Fakes/RecordingFileTreeBlobContainerService.cs @@ -0,0 +1,157 @@ +using System.Collections.Concurrent; +using Arius.Core.Shared.Storage; + +namespace Arius.Core.Tests.Shared.FileTree.Fakes; + +internal sealed class RecordingFileTreeBlobContainerService : IBlobContainerService +{ + private readonly ConcurrentDictionary _blobs = new(StringComparer.Ordinal); + private readonly TimeSpan _fileTreeUploadDelay; + private readonly TaskCompletionSource _firstFileTreeUploadStarted = new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly TaskCompletionSource _allowFileTreeUploads = new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly bool _blockFileTreeUploads; + private readonly bool _throwOnFileTreeUpload; + + private int _activeFileTreeUploads; + private int _maxConcurrentFileTreeUploads; + + public RecordingFileTreeBlobContainerService(TimeSpan? fileTreeUploadDelay = null, bool blockFileTreeUploads = false, bool throwOnFileTreeUpload = false) + { + _fileTreeUploadDelay = fileTreeUploadDelay ?? TimeSpan.Zero; + _blockFileTreeUploads = blockFileTreeUploads; + _throwOnFileTreeUpload = throwOnFileTreeUpload; + + if (!blockFileTreeUploads) + _allowFileTreeUploads.TrySetResult(); + } + + public int MaxConcurrentFileTreeUploads => Volatile.Read(ref _maxConcurrentFileTreeUploads); + public Task FirstFileTreeUploadStarted => _firstFileTreeUploadStarted.Task; + + public void AllowFileTreeUploads() => _allowFileTreeUploads.TrySetResult(); + + public Task CreateContainerIfNotExistsAsync(CancellationToken cancellationToken = default) => Task.CompletedTask; + + public async Task UploadAsync(string blobName, Stream content, IReadOnlyDictionary metadata, BlobTier tier, string? contentType = null, bool overwrite = false, CancellationToken cancellationToken = default) + { + if (!overwrite && _blobs.ContainsKey(blobName)) + throw new BlobAlreadyExistsException(blobName); + + var isFileTree = blobName.StartsWith(BlobPaths.FileTrees, StringComparison.Ordinal); + if (isFileTree) + { + _firstFileTreeUploadStarted.TrySetResult(); + var active = Interlocked.Increment(ref _activeFileTreeUploads); + UpdateMaxConcurrency(active); + await _allowFileTreeUploads.Task.WaitAsync(cancellationToken); + if (_throwOnFileTreeUpload) + throw new IOException("Simulated filetree upload failure"); + if (_fileTreeUploadDelay > TimeSpan.Zero) + await Task.Delay(_fileTreeUploadDelay, cancellationToken); + } + + try + { + await using var ms = new MemoryStream(); + await content.CopyToAsync(ms, cancellationToken); + _blobs[blobName] = new StoredBlob(ms.ToArray(), new Dictionary(metadata), tier, contentType, false); + } + finally + { + if (isFileTree) + Interlocked.Decrement(ref _activeFileTreeUploads); + } + } + + public Task OpenWriteAsync(string blobName, string? contentType = null, CancellationToken cancellationToken = default) + => Task.FromResult(new CommitOnDisposeStream(bytes => + { + _blobs[blobName] = new StoredBlob(bytes, new Dictionary(), null, contentType, false); + })); + + public Task DownloadAsync(string blobName, CancellationToken cancellationToken = default) + => Task.FromResult(new MemoryStream(_blobs[blobName].Content, writable: false)); + + public Task GetMetadataAsync(string blobName, CancellationToken cancellationToken = default) + { + if (!_blobs.TryGetValue(blobName, out var blob)) + return Task.FromResult(new BlobMetadata { Exists = false }); + + return Task.FromResult(new BlobMetadata + { + Exists = true, + Tier = blob.Tier, + ContentLength = blob.Content.LongLength, + IsRehydrating = blob.IsRehydrating, + Metadata = new Dictionary(blob.Metadata) + }); + } + + public async IAsyncEnumerable ListAsync(string prefix, [System.Runtime.CompilerServices.EnumeratorCancellation] CancellationToken cancellationToken = default) + { + foreach (var blobName in _blobs.Keys.Where(name => name.StartsWith(prefix, StringComparison.Ordinal)).OrderBy(name => name, StringComparer.Ordinal)) + { + cancellationToken.ThrowIfCancellationRequested(); + yield return blobName; + await Task.Yield(); + } + } + + public Task SetMetadataAsync(string blobName, IReadOnlyDictionary metadata, CancellationToken cancellationToken = default) + { + var blob = _blobs[blobName]; + _blobs[blobName] = blob with { Metadata = new Dictionary(metadata) }; + return Task.CompletedTask; + } + + public Task SetTierAsync(string blobName, BlobTier tier, CancellationToken cancellationToken = default) + { + var blob = _blobs[blobName]; + _blobs[blobName] = blob with { Tier = tier }; + return Task.CompletedTask; + } + + public Task CopyAsync(string sourceBlobName, string destinationBlobName, BlobTier destinationTier, RehydratePriority? rehydratePriority = null, CancellationToken cancellationToken = default) + { + var source = _blobs[sourceBlobName]; + _blobs[destinationBlobName] = source with { Tier = destinationTier, IsRehydrating = false }; + return Task.CompletedTask; + } + + public Task DeleteAsync(string blobName, CancellationToken cancellationToken = default) + { + _blobs.TryRemove(blobName, out _); + return Task.CompletedTask; + } + + private void UpdateMaxConcurrency(int active) + { + while (true) + { + var current = Volatile.Read(ref _maxConcurrentFileTreeUploads); + if (active <= current) + return; + + if (Interlocked.CompareExchange(ref _maxConcurrentFileTreeUploads, active, current) == current) + return; + } + } + + private sealed record StoredBlob( + byte[] Content, + Dictionary Metadata, + BlobTier? Tier, + string? ContentType, + bool IsRehydrating); + + private sealed class CommitOnDisposeStream(Action onCommit) : MemoryStream + { + protected override void Dispose(bool disposing) + { + if (disposing) + onCommit(ToArray()); + + base.Dispose(disposing); + } + } +} diff --git a/src/Arius.Core.Tests/Shared/FileTree/FileTreeBuilderTests.cs b/src/Arius.Core.Tests/Shared/FileTree/FileTreeBuilderTests.cs index 9dce78bc..b4be38f1 100644 --- a/src/Arius.Core.Tests/Shared/FileTree/FileTreeBuilderTests.cs +++ b/src/Arius.Core.Tests/Shared/FileTree/FileTreeBuilderTests.cs @@ -3,6 +3,7 @@ using Arius.Core.Shared.FileTree; using Arius.Core.Shared.Storage; using Arius.Core.Tests.Fakes; +using Arius.Core.Tests.Shared.FileTree.Fakes; namespace Arius.Core.Tests.Shared.FileTree; @@ -178,4 +179,116 @@ public async Task BuildAsync_DeduplicatesBlob_WhenAlreadyOnDisk() Directory.Delete(cacheDir, recursive: true); } } + + [Test] + public async Task BuildAsync_MultipleMissingTrees_UsesParallelUploads() + { + const string acct = "acct-tree-parallel"; + const string cont = "cont-tree-parallel"; + var cacheDir = FileTreeService.GetDiskCacheDirectory(acct, cont); + + if (Directory.Exists(cacheDir)) Directory.Delete(cacheDir, recursive: true); + + var manifestPath = Path.GetTempFileName(); + try + { + var now = new DateTimeOffset(2024, 6, 15, 10, 0, 0, TimeSpan.Zero); + var lines = new[] + { + new ManifestEntry("photos/2024/june/a.jpg", "hash1", now, now).Serialize(), + new ManifestEntry("photos/2024/june/b.jpg", "hash2", now, now).Serialize(), + new ManifestEntry("docs/report.pdf", "hash3", now, now).Serialize(), + }; + await File.WriteAllTextAsync(manifestPath, string.Join("\n", lines) + "\n"); + + var blobs = new RecordingFileTreeBlobContainerService(TimeSpan.FromMilliseconds(150)); + var builder = CreateBuilder(blobs, acct, cont); + + var root = await builder.BuildAsync(manifestPath); + + root.ShouldNotBeNull(); + blobs.MaxConcurrentFileTreeUploads.ShouldBeGreaterThan(1); + } + finally + { + File.Delete(manifestPath); + if (Directory.Exists(cacheDir)) Directory.Delete(cacheDir, recursive: true); + } + } + + [Test] + public async Task BuildAsync_TreeUploadProgress_ReportsCompletedUploads() + { + const string acct = "acct-tree-progress"; + const string cont = "cont-tree-progress"; + var cacheDir = FileTreeService.GetDiskCacheDirectory(acct, cont); + + if (Directory.Exists(cacheDir)) Directory.Delete(cacheDir, recursive: true); + + var manifestPath = Path.GetTempFileName(); + try + { + var now = new DateTimeOffset(2024, 6, 15, 10, 0, 0, TimeSpan.Zero); + var lines = new[] + { + new ManifestEntry("photos/2024/june/a.jpg", "hash1", now, now).Serialize(), + new ManifestEntry("photos/2024/june/b.jpg", "hash2", now, now).Serialize(), + new ManifestEntry("docs/report.pdf", "hash3", now, now).Serialize(), + }; + await File.WriteAllTextAsync(manifestPath, string.Join("\n", lines) + "\n"); + + var blobs = new RecordingFileTreeBlobContainerService(); + var builder = CreateBuilder(blobs, acct, cont); + var updates = new List<(int Completed, int Total)>(); + var progress = new SynchronousProgress<(int Completed, int Total)>(update => updates.Add(update)); + + var root = await builder.BuildAsync(manifestPath, progress); + + root.ShouldNotBeNull(); + updates.ShouldNotBeEmpty(); + updates.Select(u => u.Total).Distinct().Count().ShouldBe(1); + updates.Select(u => u.Completed).OrderBy(x => x).Last().ShouldBe(updates[0].Total); + } + finally + { + File.Delete(manifestPath); + if (Directory.Exists(cacheDir)) Directory.Delete(cacheDir, recursive: true); + } + } + + [Test] + public async Task BuildAsync_FailedUpload_DoesNotPublishFinalCacheFile() + { + const string acct = "acct-tree-failure"; + const string cont = "cont-tree-failure"; + var cacheDir = FileTreeService.GetDiskCacheDirectory(acct, cont); + + if (Directory.Exists(cacheDir)) Directory.Delete(cacheDir, recursive: true); + + var manifestPath = Path.GetTempFileName(); + try + { + var now = new DateTimeOffset(2024, 6, 15, 10, 0, 0, TimeSpan.Zero); + await File.WriteAllTextAsync(manifestPath, + new ManifestEntry("readme.txt", "aabbccdd", now, now).Serialize() + "\n"); + + var blobs = new RecordingFileTreeBlobContainerService(throwOnFileTreeUpload: true); + var builder = CreateBuilder(blobs, acct, cont); + + await Should.ThrowAsync(() => builder.BuildAsync(manifestPath)); + + Directory.Exists(cacheDir).ShouldBeTrue(); + Directory.EnumerateFiles(cacheDir).ShouldBeEmpty(); + } + finally + { + File.Delete(manifestPath); + if (Directory.Exists(cacheDir)) Directory.Delete(cacheDir, recursive: true); + } + } + + private sealed class SynchronousProgress(Action onReport) : IProgress + { + public void Report(T value) => onReport(value); + } } diff --git a/src/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cs b/src/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cs index 5e94aa68..c6e4134e 100644 --- a/src/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cs +++ b/src/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cs @@ -472,19 +472,32 @@ async Task SealCurrentTar() // ── End-of-pipeline ─────────────────────────────────────────────── - // Task 5.1: Validate the filetree service before building the tree. - await _fileTreeService.ValidateAsync(cancellationToken); + // Task 8.10 / 8.11: overlap independent finalization work. + await manifestWriter.DisposeAsync(); - // Task 8.10: Index flush - await _chunkIndex.FlushAsync(cancellationToken); - _logger.LogInformation("[index] Flush complete"); + var flushTask = Task.Run(async () => + { + var flushProgress = new Progress<(int Completed, int Total)>(update => + _mediator.Publish(new ChunkIndexFlushProgressEvent(update.Completed, update.Total), cancellationToken).AsTask().GetAwaiter().GetResult()); - // Task 8.11: Sort manifest → build tree → create snapshot - await manifestWriter.DisposeAsync(); - await ManifestSorter.SortAsync(manifestPath, cancellationToken); + await _chunkIndex.FlushAsync(flushProgress, cancellationToken); + _logger.LogInformation("[index] Flush complete"); + }, cancellationToken); + + var treeTask = Task.Run(async () => + { + await ManifestSorter.SortAsync(manifestPath, cancellationToken); + + var treeBuilder = new FileTreeBuilder(_encryption, _fileTreeService); + var treeProgress = new Progress<(int Completed, int Total)>(update => + _mediator.Publish(new TreeUploadProgressEvent(update.Completed, update.Total), cancellationToken).AsTask().GetAwaiter().GetResult()); + + return await treeBuilder.BuildAsync(manifestPath, treeProgress, cancellationToken); + }, cancellationToken); + + await Task.WhenAll(flushTask, treeTask); - var treeBuilder = new FileTreeBuilder(_encryption, _fileTreeService); - var rootHash = await treeBuilder.BuildAsync(manifestPath, cancellationToken); + var rootHash = await treeTask; _logger.LogInformation("[tree] Build complete: rootHash={RootHash}", rootHash is not null ? rootHash[..8] : "(none)"); string? snapshotRootHash = null; diff --git a/src/Arius.Core/Features/ArchiveCommand/Events.cs b/src/Arius.Core/Features/ArchiveCommand/Events.cs index 8face3c2..c8168c42 100644 --- a/src/Arius.Core/Features/ArchiveCommand/Events.cs +++ b/src/Arius.Core/Features/ArchiveCommand/Events.cs @@ -38,6 +38,12 @@ public sealed record TarBundleSealingEvent(int EntryCount, long UncompressedSize /// A tar bundle was uploaded. public sealed record TarBundleUploadedEvent(string TarHash, long CompressedSize, int EntryCount) : INotification; +/// Chunk-index flush progress update. +public sealed record ChunkIndexFlushProgressEvent(int ShardsCompleted, int TotalShards) : INotification; + +/// Filetree upload progress update. +public sealed record TreeUploadProgressEvent(int BlobsUploaded, int TotalBlobs) : INotification; + /// Snapshot creation complete. public sealed record SnapshotCreatedEvent(string RootHash, DateTimeOffset Timestamp, long FileCount) : INotification; @@ -48,4 +54,4 @@ public sealed record TarBundleStartedEvent() : INotification; /// Content hash of the file added. /// Number of entries in the current tar bundle so far. /// Cumulative uncompressed size of the current tar bundle in bytes. -public sealed record TarEntryAddedEvent(string ContentHash, int CurrentEntryCount, long CurrentTarSize) : INotification; \ No newline at end of file +public sealed record TarEntryAddedEvent(string ContentHash, int CurrentEntryCount, long CurrentTarSize) : INotification; diff --git a/src/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cs b/src/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cs index 01eff78c..0cf9bbe2 100644 --- a/src/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cs +++ b/src/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cs @@ -20,6 +20,7 @@ public sealed class ChunkIndexService : IDisposable // ── Configuration ───────────────────────────────────────────────────────── public const long DefaultCacheBudgetBytes = 512L * 1024 * 1024; // 512 MB + private const int FlushWorkers = 32; // ── Dependencies ────────────────────────────────────────────────────────── @@ -138,46 +139,57 @@ public void AddEntry(ShardEntry entry) /// Merges all pending entries into existing shards and uploads changed shards. /// Should be called once at the end of an archive run. /// - public async Task FlushAsync(CancellationToken cancellationToken = default) + public async Task FlushAsync(IProgress<(int Completed, int Total)>? progress = null, CancellationToken cancellationToken = default) { if (_pendingEntries.IsEmpty) return; - // Group pending entries by shard prefix - var byPrefix = _pendingEntries.GroupBy(e => Shard.PrefixOf(e.ContentHash)); + // Snapshot and clear pending before launching workers so this flush operation has a + // stable prefix set and new entries recorded later are left for the next flush. + var pending = new List(); + while (_pendingEntries.TryTake(out var entry)) + pending.Add(entry); - foreach (var group in byPrefix) - { - var prefix = group.Key; - var existing = await LoadShardAsync(prefix, cancellationToken); - var merged = existing.Merge(group); - - // Serialize and upload - var bytes = await ShardSerializer.SerializeAsync(merged, _encryption, cancellationToken); - var blobName = BlobPaths.ChunkIndexShard(prefix); - - await _blobs.UploadAsync( - blobName: blobName, - content: new MemoryStream(bytes), - metadata: new Dictionary(), - tier: BlobTier.Cool, - contentType: _encryption.IsEncrypted - ? ContentTypes.ChunkIndexGcmEncrypted - : ContentTypes.ChunkIndexPlaintext, - overwrite: true, - cancellationToken: cancellationToken); - - // Save to L2 disk cache (plaintext, no gzip/encryption) - SaveToL2(prefix, merged); - - // Promote merged shard to L1 - PromoteToL1(prefix, merged, bytes.Length); - } + var byPrefix = pending + .GroupBy(e => Shard.PrefixOf(e.ContentHash)) + .Select(group => (Prefix: group.Key, Entries: group.ToList())) + .ToList(); - // Clear pending - while (_pendingEntries.TryTake(out _)) - { - } + var total = byPrefix.Count; + var completed = 0; + + await Parallel.ForEachAsync( + byPrefix, + new ParallelOptions { MaxDegreeOfParallelism = FlushWorkers, CancellationToken = cancellationToken }, + async (group, ct) => + { + var existing = await LoadShardAsync(group.Prefix, ct); + var merged = existing.Merge(group.Entries); + + // Serialize and upload + var bytes = await ShardSerializer.SerializeAsync(merged, _encryption, ct); + var blobName = BlobPaths.ChunkIndexShard(group.Prefix); + + await _blobs.UploadAsync( + blobName: blobName, + content: new MemoryStream(bytes), + metadata: new Dictionary(), + tier: BlobTier.Cool, + contentType: _encryption.IsEncrypted + ? ContentTypes.ChunkIndexGcmEncrypted + : ContentTypes.ChunkIndexPlaintext, + overwrite: true, + cancellationToken: ct); + + // Save to L2 disk cache (plaintext, no gzip/encryption) + SaveToL2(group.Prefix, merged); + + // Promote merged shard to L1 + PromoteToL1(group.Prefix, merged, bytes.Length); + + var done = Interlocked.Increment(ref completed); + progress?.Report((done, total)); + }); } // ── Tier resolution ─────────────────────────────────────────────────────── diff --git a/src/Arius.Core/Shared/FileTree/FileTreeBuilder.cs b/src/Arius.Core/Shared/FileTree/FileTreeBuilder.cs index 0fa21fa8..32956a9c 100644 --- a/src/Arius.Core/Shared/FileTree/FileTreeBuilder.cs +++ b/src/Arius.Core/Shared/FileTree/FileTreeBuilder.cs @@ -1,4 +1,6 @@ using Arius.Core.Shared.Encryption; +using System.Collections.Concurrent; +using System.Threading.Channels; namespace Arius.Core.Shared.FileTree; @@ -21,6 +23,9 @@ namespace Arius.Core.Shared.FileTree; /// public sealed class FileTreeBuilder { + private const int UploadWorkers = 32; + private const int UploadQueueCapacity = 128; + private readonly IEncryptionService _encryption; private readonly FileTreeService _fileTreeService; @@ -43,126 +48,189 @@ public FileTreeBuilder( /// public async Task BuildAsync( string sortedManifestPath, + IProgress<(int Completed, int Total)>? progress, CancellationToken cancellationToken = default) { // Ensure cache is validated before calling ExistsInRemote (idempotent — no-op if already validated). await _fileTreeService.ValidateAsync(cancellationToken); - // Accumulated directory entries keyed by directory path (forward-slash, no trailing slash) - // Value: list of FileTreeEntry for that directory (files + resolved child dirs) - var dirEntries = new Dictionary>(StringComparer.Ordinal); + var uploadChannel = Channel.CreateBounded(UploadQueueCapacity); + var totalUploadsKnown = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var bufferedProgress = new ConcurrentQueue(); + var spoolPaths = new ConcurrentBag(); + var uploadsQueued = 0; + var uploadsCompleted = 0; - // Stream through sorted manifest entries - await foreach (var manifestEntry in ReadManifestAsync(sortedManifestPath, cancellationToken)) - { - var filePath = manifestEntry.Path; // e.g., "photos/2024/june/a.jpg" - var dirPath = GetDirectoryPath(filePath); // e.g., "photos/2024/june" - var name = Path.GetFileName(filePath); + var consumers = Enumerable.Range(0, UploadWorkers) + .Select(_ => Task.Run(async () => + { + await foreach (var upload in uploadChannel.Reader.ReadAllAsync(cancellationToken)) + { + try + { + var plaintext = await File.ReadAllBytesAsync(upload.SpoolPath, cancellationToken); + var tree = FileTreeBlobSerializer.Deserialize(plaintext); + await _fileTreeService.WriteAsync(upload.Hash, tree, cancellationToken); + + var done = Interlocked.Increment(ref uploadsCompleted); + if (progress is not null) + { + if (totalUploadsKnown.Task.IsCompletedSuccessfully) + progress.Report((done, totalUploadsKnown.Task.Result)); + else + bufferedProgress.Enqueue(done); + } + } + finally + { + try { File.Delete(upload.SpoolPath); } catch { /* best-effort cleanup */ } + } + } + }, cancellationToken)) + .ToArray(); - if (!dirEntries.TryGetValue(dirPath, out var list)) - dirEntries[dirPath] = list = new List(); + try + { + // Accumulated directory entries keyed by directory path (forward-slash, no trailing slash) + // Value: list of FileTreeEntry for that directory (files + resolved child dirs) + var dirEntries = new Dictionary>(StringComparer.Ordinal); - list.Add(new FileTreeEntry + // Stream through sorted manifest entries + await foreach (var manifestEntry in ReadManifestAsync(sortedManifestPath, cancellationToken)) { - Name = name, - Type = FileTreeEntryType.File, - Hash = manifestEntry.ContentHash, - Created = manifestEntry.Created, - Modified = manifestEntry.Modified - }); - } + var filePath = manifestEntry.Path; // e.g., "photos/2024/june/a.jpg" + var dirPath = GetDirectoryPath(filePath); // e.g., "photos/2024/june" + var name = Path.GetFileName(filePath); - if (dirEntries.Count == 0) return null; + if (!dirEntries.TryGetValue(dirPath, out var list)) + dirEntries[dirPath] = list = new List(); - // Ensure all intermediate directories exist in dirEntries - // (directories that only contain subdirectories, not files, would otherwise be missing) - var allDirs = new HashSet(dirEntries.Keys, StringComparer.Ordinal); - foreach (var dirPath in dirEntries.Keys.ToList()) - { - var parent = GetDirectoryPath(dirPath); - while (parent != string.Empty && !allDirs.Contains(parent)) + list.Add(new FileTreeEntry + { + Name = name, + Type = FileTreeEntryType.File, + Hash = manifestEntry.ContentHash, + Created = manifestEntry.Created, + Modified = manifestEntry.Modified + }); + } + + if (dirEntries.Count == 0) { - allDirs.Add(parent); - dirEntries[parent] = new List(); - parent = GetDirectoryPath(parent); + uploadChannel.Writer.Complete(); + totalUploadsKnown.TrySetResult(0); + return null; } - } - // Process directories bottom-up: deepest paths first (more slashes → deeper) - var sortedDirs = dirEntries.Keys - .OrderByDescending(d => d.Count(c => c == '/')) - .ThenByDescending(d => d, StringComparer.Ordinal) - .ToList(); + // Ensure all intermediate directories exist in dirEntries + // (directories that only contain subdirectories, not files, would otherwise be missing) + var allDirs = new HashSet(dirEntries.Keys, StringComparer.Ordinal); + foreach (var dirPath in dirEntries.Keys.ToList()) + { + var parent = GetDirectoryPath(dirPath); + while (parent != string.Empty && !allDirs.Contains(parent)) + { + allDirs.Add(parent); + dirEntries[parent] = new List(); + parent = GetDirectoryPath(parent); + } + } - // Map from directory path → its computed tree hash (for cascading to parents) - var dirHashMap = new Dictionary(StringComparer.Ordinal); + // Process directories bottom-up: deepest paths first (more slashes → deeper) + var sortedDirs = dirEntries.Keys + .OrderByDescending(d => d.Count(c => c == '/')) + .ThenByDescending(d => d, StringComparer.Ordinal) + .ToList(); - string? rootHash = null; + // Map from directory path → its computed tree hash (for cascading to parents) + var dirHashMap = new Dictionary(StringComparer.Ordinal); - foreach (var dirPath in sortedDirs) - { - var entries = dirEntries[dirPath]; + string? rootHash = null; - // Inject already-computed child directory entries - // (any immediate child directories of dirPath that have been processed) - foreach (var (childDirPath, childHash) in dirHashMap) + foreach (var dirPath in sortedDirs) { - var childParent = GetDirectoryPath(childDirPath); - if (childParent == dirPath) + var entries = dirEntries[dirPath]; + + // Inject already-computed child directory entries + // (any immediate child directories of dirPath that have been processed) + foreach (var (childDirPath, childHash) in dirHashMap) { - var childName = GetLastSegment(childDirPath); - entries.Add(new FileTreeEntry + var childParent = GetDirectoryPath(childDirPath); + if (childParent == dirPath) { - Name = childName + "/", - Type = FileTreeEntryType.Dir, - Hash = childHash, - Created = null, - Modified = null - }); + var childName = GetLastSegment(childDirPath); + entries.Add(new FileTreeEntry + { + Name = childName + "/", + Type = FileTreeEntryType.Dir, + Hash = childHash, + Created = null, + Modified = null + }); + } } - } - var tree = new FileTreeBlob { Entries = entries }; - var hash = FileTreeBlobSerializer.ComputeHash(tree, _encryption); + var tree = new FileTreeBlob { Entries = entries }; + var hash = FileTreeBlobSerializer.ComputeHash(tree, _encryption); + + await QueueUploadIfNeededAsync(hash, tree, uploadChannel.Writer, spoolPaths, () => uploadsQueued++, cancellationToken); - // Dedup: upload only if not already cached/known remotely - await EnsureUploadedAsync(hash, tree, cancellationToken); + dirHashMap[dirPath] = hash; - dirHashMap[dirPath] = hash; + // If this directory has no parent (empty string = root), it is the root + if (dirPath == string.Empty || !dirPath.Contains('/')) + { + // Check if there's a parent for this dir + var parent = GetDirectoryPath(dirPath); + if (parent == dirPath) // root + rootHash = hash; + // else: will be cascaded to parent when parent is processed + } + } - // If this directory has no parent (empty string = root), it is the root - if (dirPath == string.Empty || !dirPath.Contains('/')) + // The root is the directory with the shallowest path + // Find the directory closest to root (fewest slashes) + if (rootHash is null) { - // Check if there's a parent for this dir - var parent = GetDirectoryPath(dirPath); - if (parent == dirPath) // root - rootHash = hash; - // else: will be cascaded to parent when parent is processed + rootHash = await BuildRootBlobAsync(dirHashMap, dirEntries, uploadChannel.Writer, spoolPaths, () => uploadsQueued++, cancellationToken); } - } - // The root is the directory with the shallowest path - // Find the directory closest to root (fewest slashes) - if (rootHash is null) + totalUploadsKnown.TrySetResult(uploadsQueued); + while (bufferedProgress.TryDequeue(out var done)) + progress?.Report((done, uploadsQueued)); + + uploadChannel.Writer.Complete(); + await Task.WhenAll(consumers); + + return rootHash; + } + catch { - var rootDir = dirHashMap.Keys - .OrderBy(d => d.Count(c => c == '/')) - .ThenBy(d => d, StringComparer.Ordinal) - .First(); - - // Build the root tree — all immediate children of "" (root) - // If all files are in subdirectories, we need to build root blob too - rootHash = await BuildRootBlobAsync(dirHashMap, dirEntries, cancellationToken); + uploadChannel.Writer.TryComplete(); + throw; + } + finally + { + foreach (var spoolPath in spoolPaths) + { + try { File.Delete(spoolPath); } catch { /* best-effort cleanup */ } + } } - - return rootHash; } + public Task BuildAsync( + string sortedManifestPath, + CancellationToken cancellationToken = default) + => BuildAsync(sortedManifestPath, progress: null, cancellationToken); + // ── Private helpers ──────────────────────────────────────────────────────── private async Task BuildRootBlobAsync( Dictionary dirHashMap, Dictionary> dirEntries, + ChannelWriter uploadWriter, + ConcurrentBag spoolPaths, + Action onQueued, CancellationToken cancellationToken) { // Find top-level entries (directories/files whose parent is "") @@ -193,20 +261,31 @@ private async Task BuildRootBlobAsync( var rootTree = new FileTreeBlob { Entries = rootEntryList }; var rootHash = FileTreeBlobSerializer.ComputeHash(rootTree, _encryption); - await EnsureUploadedAsync(rootHash, rootTree, cancellationToken); + await QueueUploadIfNeededAsync(rootHash, rootTree, uploadWriter, spoolPaths, onQueued, cancellationToken); return rootHash; } /// - /// Uploads a tree blob via if not already present remotely. + /// Writes a tree blob to a temporary spool file for later parallel upload if it is not already + /// known remotely. /// - private async Task EnsureUploadedAsync( + private async Task QueueUploadIfNeededAsync( string treeHash, - FileTreeBlob tree, + FileTreeBlob tree, + ChannelWriter uploadWriter, + ConcurrentBag spoolPaths, + Action onQueued, CancellationToken cancellationToken) { - if (_fileTreeService.ExistsInRemote(treeHash)) return; - await _fileTreeService.WriteAsync(treeHash, tree, cancellationToken); + if (_fileTreeService.ExistsInRemote(treeHash)) + return; + + var plaintext = FileTreeBlobSerializer.Serialize(tree); + var spoolPath = Path.Combine(Path.GetTempPath(), $"arius-filetree-{Guid.NewGuid():N}.tmp"); + await File.WriteAllBytesAsync(spoolPath, plaintext, cancellationToken); + spoolPaths.Add(spoolPath); + onQueued(); + await uploadWriter.WriteAsync(new PendingTreeUpload(treeHash, spoolPath), cancellationToken); } // ── Manifest streaming ──────────────────────────────────────────────────── @@ -246,4 +325,6 @@ private static string GetLastSegment(string path) var idx = path.LastIndexOf('/'); return idx < 0 ? path : path[(idx + 1)..]; } + + private sealed record PendingTreeUpload(string Hash, string SpoolPath); } From c3083cf8eb58a3b38dc7d9d9e74ad2fb9f296e21 Mon Sep 17 00:00:00 2001 From: Wouter Van Ranst Date: Sun, 12 Apr 2026 16:13:03 +0200 Subject: [PATCH 6/8] refactor: use Parallel.ForEachAsync for finalization --- .../ArchiveCommand/ArchiveCommandHandler.cs | 26 ++++++----- .../Shared/FileTree/FileTreeBuilder.cs | 43 +++++++++---------- 2 files changed, 36 insertions(+), 33 deletions(-) diff --git a/src/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cs b/src/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cs index c6e4134e..868b7ce8 100644 --- a/src/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cs +++ b/src/Arius.Core/Features/ArchiveCommand/ArchiveCommandHandler.cs @@ -475,29 +475,33 @@ async Task SealCurrentTar() // Task 8.10 / 8.11: overlap independent finalization work. await manifestWriter.DisposeAsync(); - var flushTask = Task.Run(async () => + string? rootHash = null; + + async Task FlushFinalizationAsync(CancellationToken ct) { var flushProgress = new Progress<(int Completed, int Total)>(update => - _mediator.Publish(new ChunkIndexFlushProgressEvent(update.Completed, update.Total), cancellationToken).AsTask().GetAwaiter().GetResult()); + _mediator.Publish(new ChunkIndexFlushProgressEvent(update.Completed, update.Total), ct).AsTask().GetAwaiter().GetResult()); - await _chunkIndex.FlushAsync(flushProgress, cancellationToken); + await _chunkIndex.FlushAsync(flushProgress, ct); _logger.LogInformation("[index] Flush complete"); - }, cancellationToken); + } - var treeTask = Task.Run(async () => + async Task BuildTreeAsync(CancellationToken ct) { - await ManifestSorter.SortAsync(manifestPath, cancellationToken); + await ManifestSorter.SortAsync(manifestPath, ct); var treeBuilder = new FileTreeBuilder(_encryption, _fileTreeService); var treeProgress = new Progress<(int Completed, int Total)>(update => - _mediator.Publish(new TreeUploadProgressEvent(update.Completed, update.Total), cancellationToken).AsTask().GetAwaiter().GetResult()); + _mediator.Publish(new TreeUploadProgressEvent(update.Completed, update.Total), ct).AsTask().GetAwaiter().GetResult()); - return await treeBuilder.BuildAsync(manifestPath, treeProgress, cancellationToken); - }, cancellationToken); + rootHash = await treeBuilder.BuildAsync(manifestPath, treeProgress, ct); + } - await Task.WhenAll(flushTask, treeTask); + await Parallel.ForEachAsync( + new Func[] { FlushFinalizationAsync, BuildTreeAsync }, + new ParallelOptions { MaxDegreeOfParallelism = 2, CancellationToken = cancellationToken }, + async (work, ct) => await work(ct)); - var rootHash = await treeTask; _logger.LogInformation("[tree] Build complete: rootHash={RootHash}", rootHash is not null ? rootHash[..8] : "(none)"); string? snapshotRootHash = null; diff --git a/src/Arius.Core/Shared/FileTree/FileTreeBuilder.cs b/src/Arius.Core/Shared/FileTree/FileTreeBuilder.cs index 32956a9c..074d8c24 100644 --- a/src/Arius.Core/Shared/FileTree/FileTreeBuilder.cs +++ b/src/Arius.Core/Shared/FileTree/FileTreeBuilder.cs @@ -61,33 +61,31 @@ public FileTreeBuilder( var uploadsQueued = 0; var uploadsCompleted = 0; - var consumers = Enumerable.Range(0, UploadWorkers) - .Select(_ => Task.Run(async () => + var uploadTask = Parallel.ForEachAsync( + uploadChannel.Reader.ReadAllAsync(cancellationToken), + new ParallelOptions { MaxDegreeOfParallelism = UploadWorkers, CancellationToken = cancellationToken }, + async (upload, ct) => { - await foreach (var upload in uploadChannel.Reader.ReadAllAsync(cancellationToken)) + try { - try - { - var plaintext = await File.ReadAllBytesAsync(upload.SpoolPath, cancellationToken); - var tree = FileTreeBlobSerializer.Deserialize(plaintext); - await _fileTreeService.WriteAsync(upload.Hash, tree, cancellationToken); + var plaintext = await File.ReadAllBytesAsync(upload.SpoolPath, ct); + var tree = FileTreeBlobSerializer.Deserialize(plaintext); + await _fileTreeService.WriteAsync(upload.Hash, tree, ct); - var done = Interlocked.Increment(ref uploadsCompleted); - if (progress is not null) - { - if (totalUploadsKnown.Task.IsCompletedSuccessfully) - progress.Report((done, totalUploadsKnown.Task.Result)); - else - bufferedProgress.Enqueue(done); - } - } - finally + var done = Interlocked.Increment(ref uploadsCompleted); + if (progress is not null) { - try { File.Delete(upload.SpoolPath); } catch { /* best-effort cleanup */ } + if (totalUploadsKnown.Task.IsCompletedSuccessfully) + progress.Report((done, totalUploadsKnown.Task.Result)); + else + bufferedProgress.Enqueue(done); } } - }, cancellationToken)) - .ToArray(); + finally + { + try { File.Delete(upload.SpoolPath); } catch { /* best-effort cleanup */ } + } + }); try { @@ -119,6 +117,7 @@ public FileTreeBuilder( { uploadChannel.Writer.Complete(); totalUploadsKnown.TrySetResult(0); + await uploadTask; return null; } @@ -200,7 +199,7 @@ public FileTreeBuilder( progress?.Report((done, uploadsQueued)); uploadChannel.Writer.Complete(); - await Task.WhenAll(consumers); + await uploadTask; return rootHash; } From b6431fc09fd58325df10fcf418f9baec88d0dbab Mon Sep 17 00:00:00 2001 From: Wouter Van Ranst Date: Sun, 12 Apr 2026 16:27:51 +0200 Subject: [PATCH 7/8] fix: preserve pending chunk-index entries on flush failure --- .../ChunkIndex/ChunkIndexServiceTests.cs | 52 +++++++++++- ...RecordingChunkIndexBlobContainerService.cs | 33 +++++--- .../Shared/ChunkIndex/ChunkIndexService.cs | 81 ++++++++++++------- 3 files changed, 122 insertions(+), 44 deletions(-) diff --git a/src/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexServiceTests.cs b/src/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexServiceTests.cs index 9a54b802..1977e2f2 100644 --- a/src/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexServiceTests.cs +++ b/src/Arius.Core.Tests/Shared/ChunkIndex/ChunkIndexServiceTests.cs @@ -2,6 +2,7 @@ using Arius.Core.Shared.ChunkIndex; using Arius.Core.Shared.Encryption; using Arius.Core.Tests.Shared.ChunkIndex.Fakes; +using System.Collections.Concurrent; using TUnit.Core; namespace Arius.Core.Tests.Shared.ChunkIndex; @@ -100,8 +101,8 @@ public async Task FlushAsync_ReportsProgress_PerCompletedPrefix() { var blobs = new RecordingChunkIndexBlobContainerService(); var service = new ChunkIndexService(blobs, s_encryption, account, container); - var updates = new List<(int Completed, int Total)>(); - var progress = new SynchronousProgress<(int Completed, int Total)>(update => updates.Add(update)); + var updates = new ConcurrentQueue<(int Completed, int Total)>(); + var progress = new SynchronousProgress<(int Completed, int Total)>(update => updates.Enqueue(update)); service.AddEntry(new ShardEntry("aaaa" + new string('1', 60), "chunk-a", 10, 5)); service.AddEntry(new ShardEntry("bbbb" + new string('2', 60), "chunk-b", 20, 8)); @@ -119,6 +120,45 @@ public async Task FlushAsync_ReportsProgress_PerCompletedPrefix() } } + [Test] + public async Task FlushAsync_WhenPrefixUploadFails_RequeuesPendingEntries() + { + const string account = "acct-ci-requeue"; + var container = $"ctr-ci-requeue-{Guid.NewGuid():N}"; + CleanupRepo(account, container); + + try + { + var failingPrefix = "bbbb"; + var blobs = new RecordingChunkIndexBlobContainerService(failUploadForPrefix: failingPrefix); + var service = new ChunkIndexService(blobs, s_encryption, account, container); + + var firstPrefixEntry = new ShardEntry("aaaa" + new string('1', 60), "chunk-a", 10, 5); + var failingPrefixEntry = new ShardEntry(failingPrefix + new string('2', 60), "chunk-b", 20, 8); + + service.AddEntry(firstPrefixEntry); + service.AddEntry(failingPrefixEntry); + + await Should.ThrowAsync(() => service.FlushAsync()); + + blobs.ClearFailure(); + + await service.FlushAsync(); + + var firstLookup = await service.LookupAsync(firstPrefixEntry.ContentHash); + var secondLookup = await service.LookupAsync(failingPrefixEntry.ContentHash); + + firstLookup.ShouldNotBeNull(); + firstLookup.ChunkHash.ShouldBe(firstPrefixEntry.ChunkHash); + secondLookup.ShouldNotBeNull(); + secondLookup.ChunkHash.ShouldBe(failingPrefixEntry.ChunkHash); + } + finally + { + CleanupRepo(account, container); + } + } + private static void CleanupRepo(string account, string container) { var repoDir = RepositoryPaths.GetRepositoryDirectory(account, container); @@ -128,6 +168,12 @@ private static void CleanupRepo(string account, string container) private sealed class SynchronousProgress(Action onReport) : IProgress { - public void Report(T value) => onReport(value); + private readonly Lock _lock = new(); + + public void Report(T value) + { + lock (_lock) + onReport(value); + } } } diff --git a/src/Arius.Core.Tests/Shared/ChunkIndex/Fakes/RecordingChunkIndexBlobContainerService.cs b/src/Arius.Core.Tests/Shared/ChunkIndex/Fakes/RecordingChunkIndexBlobContainerService.cs index 50a9f6de..161ce2a5 100644 --- a/src/Arius.Core.Tests/Shared/ChunkIndex/Fakes/RecordingChunkIndexBlobContainerService.cs +++ b/src/Arius.Core.Tests/Shared/ChunkIndex/Fakes/RecordingChunkIndexBlobContainerService.cs @@ -7,21 +7,25 @@ internal sealed class RecordingChunkIndexBlobContainerService : IBlobContainerSe { private readonly ConcurrentDictionary _blobs = new(StringComparer.Ordinal); private readonly TimeSpan _chunkIndexUploadDelay; + private string? _failUploadForPrefix; private int _activeChunkIndexUploads; private int _maxConcurrentChunkIndexUploads; private int _chunkIndexMetadataReads; private int _chunkIndexDownloads; - public RecordingChunkIndexBlobContainerService(TimeSpan? chunkIndexUploadDelay = null) + public RecordingChunkIndexBlobContainerService(TimeSpan? chunkIndexUploadDelay = null, string? failUploadForPrefix = null) { _chunkIndexUploadDelay = chunkIndexUploadDelay ?? TimeSpan.Zero; + _failUploadForPrefix = failUploadForPrefix; } public int MaxConcurrentChunkIndexUploads => Volatile.Read(ref _maxConcurrentChunkIndexUploads); public int ChunkIndexMetadataReads => Volatile.Read(ref _chunkIndexMetadataReads); public int ChunkIndexDownloads => Volatile.Read(ref _chunkIndexDownloads); + public void ClearFailure() => _failUploadForPrefix = null; + public Task CreateContainerIfNotExistsAsync(CancellationToken cancellationToken = default) => Task.CompletedTask; public async Task UploadAsync(string blobName, Stream content, IReadOnlyDictionary metadata, BlobTier tier, string? contentType = null, bool overwrite = false, CancellationToken cancellationToken = default) @@ -31,21 +35,30 @@ public async Task UploadAsync(string blobName, Stream content, IReadOnlyDictiona { var active = Interlocked.Increment(ref _activeChunkIndexUploads); UpdateMaxConcurrency(active); - if (_chunkIndexUploadDelay > TimeSpan.Zero) - await Task.Delay(_chunkIndexUploadDelay, cancellationToken); + try + { + if (_chunkIndexUploadDelay > TimeSpan.Zero) + await Task.Delay(_chunkIndexUploadDelay, cancellationToken); + + if (_failUploadForPrefix is not null && blobName == BlobPaths.ChunkIndexShard(_failUploadForPrefix)) + throw new IOException($"Simulated chunk-index upload failure for prefix {_failUploadForPrefix}."); + + await using var ms = new MemoryStream(); + await content.CopyToAsync(ms, cancellationToken); + _blobs[blobName] = new StoredBlob(ms.ToArray(), new Dictionary(metadata), tier, contentType, false); + } + finally + { + var remaining = Interlocked.Decrement(ref _activeChunkIndexUploads); + UpdateMaxConcurrency(remaining); + } } - - try + else { await using var ms = new MemoryStream(); await content.CopyToAsync(ms, cancellationToken); _blobs[blobName] = new StoredBlob(ms.ToArray(), new Dictionary(metadata), tier, contentType, false); } - finally - { - if (isChunkIndex) - Interlocked.Decrement(ref _activeChunkIndexUploads); - } } public Task OpenWriteAsync(string blobName, string? contentType = null, CancellationToken cancellationToken = default) diff --git a/src/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cs b/src/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cs index 0cf9bbe2..b6b44a3b 100644 --- a/src/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cs +++ b/src/Arius.Core/Shared/ChunkIndex/ChunkIndexService.cs @@ -158,38 +158,57 @@ public async Task FlushAsync(IProgress<(int Completed, int Total)>? progress = n var total = byPrefix.Count; var completed = 0; - await Parallel.ForEachAsync( - byPrefix, - new ParallelOptions { MaxDegreeOfParallelism = FlushWorkers, CancellationToken = cancellationToken }, - async (group, ct) => + var flushedPrefixes = new ConcurrentDictionary(StringComparer.Ordinal); + + try + { + await Parallel.ForEachAsync( + byPrefix, + new ParallelOptions { MaxDegreeOfParallelism = FlushWorkers, CancellationToken = cancellationToken }, + async (group, ct) => + { + var existing = await LoadShardAsync(group.Prefix, ct); + var merged = existing.Merge(group.Entries); + + // Serialize and upload + var bytes = await ShardSerializer.SerializeAsync(merged, _encryption, ct); + var blobName = BlobPaths.ChunkIndexShard(group.Prefix); + + await _blobs.UploadAsync( + blobName: blobName, + content: new MemoryStream(bytes), + metadata: new Dictionary(), + tier: BlobTier.Cool, + contentType: _encryption.IsEncrypted + ? ContentTypes.ChunkIndexGcmEncrypted + : ContentTypes.ChunkIndexPlaintext, + overwrite: true, + cancellationToken: ct); + + // Save to L2 disk cache (plaintext, no gzip/encryption) + SaveToL2(group.Prefix, merged); + + // Promote merged shard to L1 + PromoteToL1(group.Prefix, merged, bytes.Length); + flushedPrefixes.TryAdd(group.Prefix, 0); + + var done = Interlocked.Increment(ref completed); + progress?.Report((done, total)); + }); + } + catch + { + foreach (var group in byPrefix) { - var existing = await LoadShardAsync(group.Prefix, ct); - var merged = existing.Merge(group.Entries); - - // Serialize and upload - var bytes = await ShardSerializer.SerializeAsync(merged, _encryption, ct); - var blobName = BlobPaths.ChunkIndexShard(group.Prefix); - - await _blobs.UploadAsync( - blobName: blobName, - content: new MemoryStream(bytes), - metadata: new Dictionary(), - tier: BlobTier.Cool, - contentType: _encryption.IsEncrypted - ? ContentTypes.ChunkIndexGcmEncrypted - : ContentTypes.ChunkIndexPlaintext, - overwrite: true, - cancellationToken: ct); - - // Save to L2 disk cache (plaintext, no gzip/encryption) - SaveToL2(group.Prefix, merged); - - // Promote merged shard to L1 - PromoteToL1(group.Prefix, merged, bytes.Length); - - var done = Interlocked.Increment(ref completed); - progress?.Report((done, total)); - }); + if (flushedPrefixes.ContainsKey(group.Prefix)) + continue; + + foreach (var entry in group.Entries) + _pendingEntries.Add(entry); + } + + throw; + } } // ── Tier resolution ─────────────────────────────────────────────────────── From 747f39d8dd52c48db2f557d9f012d7ea8ef187ad Mon Sep 17 00:00:00 2001 From: Wouter Van Ranst Date: Sun, 12 Apr 2026 16:33:04 +0200 Subject: [PATCH 8/8] chore: revert README changes --- README.md | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/README.md b/README.md index d096ab4d..686442d2 100644 --- a/README.md +++ b/README.md @@ -101,20 +101,6 @@ dotnet user-secrets set "arius::key" "" Most test projects can be run directly with `dotnet test --project `. `src/Arius.E2E.Tests` also requires `ARIUS_E2E_ACCOUNT` and `ARIUS_E2E_KEY` to be set; otherwise the suite fails immediately with a configuration error. -## Archive Finalization - -Archive writes a temporary `archive manifest` on disk, externally sorts it, builds/uploads -filetrees, flushes chunk-index shards, and only then creates the snapshot. - -Chunk-index flush and filetree upload now run in parallel during finalization, but snapshot -creation is still the only repository commit point. If a run crashes after some shard or -filetree uploads but before the snapshot is written, that work is treated as incomplete -state rather than a completed archive. - -To keep the domain language precise, use `archive manifest` for the temporary tree-build -input file and `snapshot` or `snapshot manifest` for the durable repository state stored -under `snapshots/`. - ## Updating Run: