Add instance lock service #1582
Conversation
📝 WalkthroughWalkthroughAdds instance-level locking: new InstanceLockClient, InstanceLocker service and DI, ProcessEngine lock invocation, telemetry helpers, HttpClient.PatchAsync extension, sanitized PlatformHttpResponseSnapshotException factory, tests (unit/integration) and package version bump. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/Altinn.App.Api/Infrastructure/Middleware/EnableProcessLockAttribute.cs(1 hunks)src/Altinn.App.Api/Infrastructure/Middleware/ProcessLockMiddleware.cs(1 hunks)src/Altinn.App.Api/Infrastructure/Middleware/ProcessLockOptions.cs(1 hunks)src/Altinn.App.Core/Extensions/HttpClientExtension.cs(1 hunks)src/Altinn.App.Core/Features/Telemetry/Telemetry.ProcessLockClient.cs(1 hunks)src/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs(1 hunks)src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLockRequest.cs(1 hunks)src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLockResponse.cs(1 hunks)test/Altinn.App.Api.Tests/Altinn.App.Api.Tests.csproj(1 hunks)test/Altinn.App.Api.Tests/Controllers/Conventions/EnumSerializationTests.cs(1 hunks)test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
test/Altinn.App.Api.Tests/Controllers/Conventions/EnumSerializationTests.cstest/Altinn.App.Api.Tests/Altinn.App.Api.Tests.csprojsrc/Altinn.App.Core/Extensions/HttpClientExtension.cssrc/Altinn.App.Api/Infrastructure/Middleware/EnableProcessLockAttribute.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
test/Altinn.App.Api.Tests/Altinn.App.Api.Tests.csprojtest/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
test/Altinn.App.Api.Tests/Altinn.App.Api.Tests.csproj
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Api.Tests/Altinn.App.Api.Tests.csprojtest/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.cs
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Api.Tests/Altinn.App.Api.Tests.csproj
📚 Learning: 2025-09-07T20:03:48.030Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1463
File: test/Altinn.App.SourceGenerator.Tests/DiagnosticTests.RunJsonError.verified.txt:1-21
Timestamp: 2025-09-07T20:03:48.030Z
Learning: In Altinn.App.SourceGenerator.Tests, the path C:\temp\config\applicationmetadata.json is a hardcoded test fixture path required by Roslyn and is used consistently across all operating systems, not an actual filesystem path that varies by OS.
Applied to files:
test/Altinn.App.Api.Tests/Altinn.App.Api.Tests.csprojtest/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.cs
🧬 Code graph analysis (5)
src/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs (5)
src/Altinn.App.Core/Configuration/PlatformSettings.cs (1)
PlatformSettings(7-67)src/Altinn.App.Core/Constants/General.cs (1)
General(6-42)src/Altinn.App.Core/Extensions/HttpClientExtension.cs (6)
Task(20-43)Task(55-78)Task(89-110)Task(124-145)Task(157-180)Task(191-212)src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLockRequest.cs (1)
ProcessLockRequest(3-6)src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLockResponse.cs (1)
ProcessLockResponse(3-6)
src/Altinn.App.Core/Features/Telemetry/Telemetry.ProcessLockClient.cs (2)
src/Altinn.App.Core/Features/Signing/Services/SigningDelegationService.cs (1)
Guid(146-156)src/Altinn.App.Api/Infrastructure/Middleware/ProcessLockMiddleware.cs (1)
instanceOwnerPartyId(92-107)
test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.cs (4)
src/Altinn.App.Core/Configuration/PlatformSettings.cs (1)
PlatformSettings(7-67)src/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs (4)
ProcessLockClient(15-99)ProcessLockClient(22-37)Task(39-78)Task(80-98)src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLockResponse.cs (1)
ProcessLockResponse(3-6)src/Altinn.App.Api/Infrastructure/Middleware/ProcessLockOptions.cs (1)
ProcessLockOptions(3-6)
src/Altinn.App.Core/Extensions/HttpClientExtension.cs (3)
src/Altinn.App.Core/Infrastructure/Clients/AccessManagement/AccessManagementClient.cs (1)
HttpRequestMessage(132-142)src/Altinn.App.Core/Constants/AuthorizationSchemes.cs (1)
AuthorizationSchemes(6-12)src/Altinn.App.Core/Constants/General.cs (1)
General(6-42)
src/Altinn.App.Api/Infrastructure/Middleware/ProcessLockMiddleware.cs (2)
src/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs (4)
ProcessLockClient(15-99)ProcessLockClient(22-37)Task(39-78)Task(80-98)src/Altinn.App.Api/Infrastructure/Middleware/ProcessLockOptions.cs (1)
ProcessLockOptions(3-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
🔇 Additional comments (13)
src/Altinn.App.Api/Infrastructure/Middleware/EnableProcessLockAttribute.cs (1)
1-4: LGTM!Clean marker attribute implementation following standard .NET patterns. The attribute configuration is appropriate for marking endpoints that require process locking.
test/Altinn.App.Api.Tests/Altinn.App.Api.Tests.csproj (1)
21-21: LGTM!WireMock.Net is an appropriate choice for mocking HTTP interactions in the new ProcessLockMiddleware tests.
src/Altinn.App.Api/Infrastructure/Middleware/ProcessLockOptions.cs (1)
1-6: LGTM!Clean implementation of the options pattern with a sensible default expiration of 5 minutes for process locks.
src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLockRequest.cs (1)
1-6: LGTM!Simple, well-defined DTO for process lock request payloads.
src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLockResponse.cs (1)
1-6: LGTM!Clean DTO implementation with appropriate use of Guid for the lock identifier.
src/Altinn.App.Core/Extensions/HttpClientExtension.cs (1)
147-180: LGTM! Public API addition.The implementation follows the established pattern of existing extension methods (PostAsync, PutAsync, etc.). As per project guidelines, this public API addition is acceptable provided it's documented in the release notes.
Based on learnings, source-compatible public API changes are acceptable when documented in release notes.
src/Altinn.App.Core/Features/Telemetry/Telemetry.ProcessLockClient.cs (1)
9-25: LGTM!Both
StartAcquireProcessLockActivityandStartReleaseProcessLockActivityare well-implemented telemetry helpers that follow consistent patterns and properly configure activity context with instance identifiers.src/Altinn.App.Api/Infrastructure/Middleware/ProcessLockMiddleware.cs (2)
18-89: Lock acquisition/release flow inInvokelooks correct and robustThe attribute check, instance id extraction,
PlatformHttpExceptionhandling (ProblemDetails with propagated status), and unconditional release infinallyall line up well with the intended behavior and the tests (including exception and failure cases). I don’t see correctness issues here.
92-119: Route data parsing and logging helpers are clear and appropriately strictRejecting requests with misconfigured routes via
InvalidOperationExceptionis reasonable for a dev/config error, and the structuredLoggerMessage-based helpers for acquire/release/failure give good observability without cluttering the main control flow. No changes needed from my side.src/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs (2)
22-37: HttpClient configuration in constructor is consistent with other platform clientsSetting
BaseAddress, subscription key header, and JSONAccepthere matches the usual Altinn client pattern and works well with theAddHttpClient<ProcessLockClient>()registration from tests. Looks good.
39-98: Acquire/Release semantics and validation are solidThe request URLs, token extraction, expiration handling, and strict validation of
ProcessLockResponse.LockId(including null/empty cases) all look correct and align with the middleware tests, including the error-path expectations. Releasing withExpiration = 0via PATCH is clear and the telemetry hooks are neatly wrapped.test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.cs (2)
23-127: Fixture wiring for TestServer and WireMock is well-structuredThe embedded
Fixturecleanly encapsulates host + mock server setup, including storage endpoint rewiring, runtime cookie name, and typedProcessLockClientregistration. The acquire/releaseIRequestBuilderhelpers keep the individual tests focused and readable. No issues spotted.
129-387: Test suite gives strong coverage of middleware behavior and edge casesThe tests exercise all key scenarios: attribute gating, normal flow, exception flow, custom expiration propagation, missing route parameters, release failures being non-fatal, storage API error mapping, and malformed/empty lock responses. Assertions against WireMock logs ensure the correct calls and payloads are sent. This provides good confidence in the new behavior.
src/Altinn.App.Core/Features/Telemetry/Telemetry.ProcessLockClient.cs
Outdated
Show resolved
Hide resolved
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
src/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs
Outdated
Show resolved
Hide resolved
test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.cs
Outdated
Show resolved
Hide resolved
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs
Fixed
Show fixed
Hide fixed
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs (1)
96-101: Ensure HttpResponseMessage is disposed to prevent resource leaks.Similar to
AcquireProcessLock, theHttpResponseMessagefrom line 96 is not explicitly disposed, which can lead to socket exhaustion.Apply the same
usingpattern as suggested forAcquireProcessLock:- var response = await _client.PatchAsync(token, apiUrl, content); + using var response = await _client.PatchAsync(token, apiUrl, content); if (!response.IsSuccessStatusCode) { throw await PlatformHttpException.CreateAsync(response); }
🧹 Nitpick comments (1)
test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.cs (1)
41-46: Clarify assumption aboutPlatformSettings.ApiStorageEndpointshapeThis line assumes that
settings.ApiStorageEndpointis already configured as an absolute URI so thatnew Uri(settings.ApiStorageEndpoint).PathAndQueryis valid. That’s fine today, but if the options default ever changes (e.g., tonullor a relative path), these tests will start failing with an exception.Consider making this assumption explicit in a comment (or guarding it) to make the intent clearer to future maintainers:
- services.Configure<PlatformSettings>(settings => - { - var testUrl = server.Url ?? throw new Exception("Missing server URL"); - settings.ApiStorageEndpoint = - testUrl + new Uri(settings.ApiStorageEndpoint).PathAndQuery; - }); + services.Configure<PlatformSettings>(settings => + { + var testUrl = server.Url ?? throw new Exception("Missing server URL"); + + // Assumes ApiStorageEndpoint is configured as an absolute URI so we can reuse its path here. + settings.ApiStorageEndpoint = + testUrl + new Uri(settings.ApiStorageEndpoint).PathAndQuery; + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/Altinn.App.Core/Features/Telemetry/Telemetry.ProcessLockClient.cs(1 hunks)src/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs(1 hunks)test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.CustomExpirationConfiguration_UsedInStorageApiCall.verified.txt(1 hunks)test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.EmptyJsonResponseBody_ReturnsProblemDetails.verified.txt(1 hunks)test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.MissingRouteParameters_ThrowsException.verified.txt(1 hunks)test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.NullResponseBody_ReturnsProblemDetails.verified.txt(1 hunks)test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.StorageApiError_ReturnsCorrectStatusCode_storageStatusCode=Conflict.verified.txt(1 hunks)test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.StorageApiError_ReturnsCorrectStatusCode_storageStatusCode=InternalServerError.verified.txt(1 hunks)test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.StorageApiError_ReturnsCorrectStatusCode_storageStatusCode=NotFound.verified.txt(1 hunks)test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.cs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.MissingRouteParameters_ThrowsException.verified.txt
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.StorageApiError_ReturnsCorrectStatusCode_storageStatusCode=NotFound.verified.txttest/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.StorageApiError_ReturnsCorrectStatusCode_storageStatusCode=InternalServerError.verified.txttest/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.StorageApiError_ReturnsCorrectStatusCode_storageStatusCode=Conflict.verified.txttest/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.CustomExpirationConfiguration_UsedInStorageApiCall.verified.txt
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.StorageApiError_ReturnsCorrectStatusCode_storageStatusCode=NotFound.verified.txttest/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.StorageApiError_ReturnsCorrectStatusCode_storageStatusCode=InternalServerError.verified.txttest/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.StorageApiError_ReturnsCorrectStatusCode_storageStatusCode=Conflict.verified.txttest/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.CustomExpirationConfiguration_UsedInStorageApiCall.verified.txt
📚 Learning: 2025-09-18T10:12:21.841Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1357
File: src/Altinn.App.Api/Infrastructure/Middleware/ScopeAuthorizationMiddleware.cs:289-293
Timestamp: 2025-09-18T10:12:21.841Z
Learning: In Altinn App security middleware like ScopeAuthorizationMiddleware, prefer explicit failures (throwing exceptions) over silently skipping unexpected endpoint types during discovery to prevent potential authorization gaps and ensure all endpoints are properly handled according to security configuration.
Applied to files:
test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.StorageApiError_ReturnsCorrectStatusCode_storageStatusCode=NotFound.verified.txttest/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.NullResponseBody_ReturnsProblemDetails.verified.txttest/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.StorageApiError_ReturnsCorrectStatusCode_storageStatusCode=InternalServerError.verified.txtsrc/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs
📚 Learning: 2025-09-07T20:03:48.030Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1463
File: test/Altinn.App.SourceGenerator.Tests/DiagnosticTests.RunJsonError.verified.txt:1-21
Timestamp: 2025-09-07T20:03:48.030Z
Learning: In Altinn.App.SourceGenerator.Tests, the path C:\temp\config\applicationmetadata.json is a hardcoded test fixture path required by Roslyn and is used consistently across all operating systems, not an actual filesystem path that varies by OS.
Applied to files:
test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.StorageApiError_ReturnsCorrectStatusCode_storageStatusCode=Conflict.verified.txttest/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.cs
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
src/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs
🧬 Code graph analysis (1)
src/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs (7)
src/Altinn.App.Core/Features/Telemetry/Telemetry.ProcessLockClient.cs (1)
Telemetry(5-24)src/Altinn.App.Core/Configuration/PlatformSettings.cs (1)
PlatformSettings(7-67)src/Altinn.App.Core/Constants/General.cs (1)
General(6-42)src/Altinn.App.Api/Infrastructure/Middleware/ProcessLockMiddleware.cs (2)
Task(31-90)instanceOwnerPartyId(92-107)src/Altinn.App.Core/Extensions/HttpClientExtension.cs (6)
Task(20-43)Task(55-78)Task(89-110)Task(124-145)Task(157-180)Task(191-212)src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLockRequest.cs (1)
ProcessLockRequest(3-6)src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLockResponse.cs (1)
ProcessLockResponse(3-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
🔇 Additional comments (3)
src/Altinn.App.Core/Features/Telemetry/Telemetry.ProcessLockClient.cs (1)
7-23: LGTM! Clean telemetry implementation.Both helper methods follow the established telemetry patterns in the codebase, correctly initialize activities with instance context, and appropriately return nullable Activity references to support optional telemetry.
src/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs (1)
24-41: Constructor properly configures the storage client.The HttpClient is correctly initialized with the storage endpoint, subscription key, and JSON accept headers. The optional telemetry parameter appropriately supports scenarios where telemetry is not available.
test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.cs (1)
19-125: Solid test harness and coverage forProcessLockMiddlewareThe in-memory host + WireMock fixture and the suite of happy-path and failure-path tests give very good coverage of the middleware contract (attribute routing, lock acquire/release, exception behavior, storage error mapping, and malformed responses). I don’t see correctness or reliability issues here; this looks ready to ship as-is from a testing perspective.
Also applies to: 127-366
src/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs
Outdated
Show resolved
Hide resolved
|
Isn't it easier to discover and more explicit to just use the You could either have an |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/Altinn.App.Core/Helpers/PlatformHttpResponseSnapshotException.cs (1)
108-116: Consider documenting the TrailingHeaders precondition.Accessing
response.TrailingHeaders(line 115, 144) before the response body is fully consumed can throwInvalidOperationExceptionin HTTP/2+ scenarios. The internal callerCreateAndDisposeHttpResponsehandles this by reading content first, but direct callers ofCreateneed to be aware of this requirement.Since the class is
internal, risk is limited, but adding a brief note in the XML doc would help future maintainers./// <param name="message">The exception message.</param> - /// <param name="response">The HTTP response to snapshot.</param> + /// <param name="response">The HTTP response to snapshot. Response content should be consumed first if trailing headers are expected.</param> /// <param name="content">The response body content as a string (possibly truncated).</param>src/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs (2)
62-71: Catch specific exceptions instead of all exceptions.The generic
catch (Exception e)clause catches all exceptions, including catastrophic ones likeOutOfMemoryException. While the exception is logged (addressing the earlier review comment), best practice is to catch only the specific exceptions you expect and can handle.Consider this refactor:
Guid? lockId = null; try { var lockResponse = await response.Content.ReadFromJsonAsync<ProcessLockResponse>(); lockId = lockResponse?.LockId; } -catch (Exception e) +catch (JsonException e) { _logger.LogError(e, "Error reading response from the lock acquisition endpoint."); } +catch (NotSupportedException e) +{ + _logger.LogError(e, "Error reading response from the lock acquisition endpoint."); +}This avoids catching unexpected exceptions while still handling the deserialization errors you anticipate.
84-102: LGTM! Consider adding CancellationToken support.The release logic is correct and properly disposes the response. The use of PATCH with
Expiration = 0to release the lock is appropriate per the storage API design.Consider adding
CancellationTokensupport to bothAcquireProcessLockandReleaseProcessLockmethods to allow cancellation propagation:-public async Task<Guid> AcquireProcessLock(Guid instanceGuid, int instanceOwnerPartyId, TimeSpan expiration) +public async Task<Guid> AcquireProcessLock(Guid instanceGuid, int instanceOwnerPartyId, TimeSpan expiration, CancellationToken cancellationToken = default) { // ... - using var response = await _client.PostAsync(token, apiUrl, content); + using var response = await _client.PostAsync(token, apiUrl, content, cancellationToken: cancellationToken); // ... } -public async Task ReleaseProcessLock(Guid instanceGuid, int instanceOwnerPartyId, Guid lockId) +public async Task ReleaseProcessLock(Guid instanceGuid, int instanceOwnerPartyId, Guid lockId, CancellationToken cancellationToken = default) { // ... - using var response = await _client.PatchAsync(token, apiUrl, content); + using var response = await _client.PatchAsync(token, apiUrl, content, cancellationToken: cancellationToken); // ... }This would allow callers to cancel long-running lock operations when needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Altinn.App.Core/Helpers/PlatformHttpResponseSnapshotException.cs(1 hunks)src/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs(1 hunks)test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.StorageApiError_ReturnsCorrectStatusCode_storageStatusCode=Conflict.verified.txt(1 hunks)test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.StorageApiError_ReturnsCorrectStatusCode_storageStatusCode=InternalServerError.verified.txt(1 hunks)test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.StorageApiError_ReturnsCorrectStatusCode_storageStatusCode=NotFound.verified.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.StorageApiError_ReturnsCorrectStatusCode_storageStatusCode=Conflict.verified.txt
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.StorageApiError_ReturnsCorrectStatusCode_storageStatusCode=NotFound.verified.txttest/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.StorageApiError_ReturnsCorrectStatusCode_storageStatusCode=InternalServerError.verified.txt
📚 Learning: 2025-09-18T10:12:21.841Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1357
File: src/Altinn.App.Api/Infrastructure/Middleware/ScopeAuthorizationMiddleware.cs:289-293
Timestamp: 2025-09-18T10:12:21.841Z
Learning: In Altinn App security middleware like ScopeAuthorizationMiddleware, prefer explicit failures (throwing exceptions) over silently skipping unexpected endpoint types during discovery to prevent potential authorization gaps and ensure all endpoints are properly handled according to security configuration.
Applied to files:
test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.StorageApiError_ReturnsCorrectStatusCode_storageStatusCode=NotFound.verified.txttest/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.StorageApiError_ReturnsCorrectStatusCode_storageStatusCode=InternalServerError.verified.txtsrc/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.StorageApiError_ReturnsCorrectStatusCode_storageStatusCode=NotFound.verified.txttest/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.StorageApiError_ReturnsCorrectStatusCode_storageStatusCode=InternalServerError.verified.txt
📚 Learning: 2025-09-25T08:15:25.624Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Applied to files:
test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.StorageApiError_ReturnsCorrectStatusCode_storageStatusCode=NotFound.verified.txtsrc/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
src/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs
🧬 Code graph analysis (1)
src/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs (6)
src/Altinn.App.Core/Features/Telemetry/Telemetry.ProcessLockClient.cs (1)
Telemetry(5-24)src/Altinn.App.Core/Configuration/PlatformSettings.cs (1)
PlatformSettings(7-67)src/Altinn.App.Core/Helpers/PlatformHttpResponseSnapshotException.cs (5)
Task(65-96)Task(207-275)PlatformHttpResponseSnapshotException(18-322)PlatformHttpResponseSnapshotException(108-166)PlatformHttpResponseSnapshotException(179-197)src/Altinn.App.Core/Extensions/HttpClientExtension.cs (6)
Task(20-43)Task(55-78)Task(89-110)Task(124-145)Task(157-180)Task(191-212)src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLockRequest.cs (1)
ProcessLockRequest(3-6)src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLockResponse.cs (1)
ProcessLockResponse(3-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
🔇 Additional comments (5)
src/Altinn.App.Core/Helpers/PlatformHttpResponseSnapshotException.cs (2)
83-95: LGTM - Correct disposal pattern with try-finally.The refactoring properly ensures the response is disposed even when snapshot creation fails, and swallowing disposal exceptions prevents masking the original exception.
119-154: Clean implementation of sanitized response construction.The header copying with consistent redaction, null-safe access patterns, and proper Content-Type preservation are well-implemented. The approach of creating a non-streaming
StringContentsnapshot ensures the exception holds no live network resources.src/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs (3)
16-41: LGTM! Constructor properly configures typed HttpClient.The constructor correctly sets up the HttpClient with base address and required headers. Modifying the injected HttpClient is appropriate for the typed client pattern where each instance receives its own HttpClient.
43-60: Resource disposal properly handled.The use of
using var responseat line 55 ensures the HttpResponseMessage is properly disposed. The previous concern about resource leaks has been addressed.
73-82: LGTM! Lock validation logic is correct.The validation properly checks for null or empty GUID and throws an appropriate exception with response details. The
usingstatement ensures the response is disposed after the exception is created.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
test/Altinn.App.Integration.Tests/ProcessLock/ProcessLockTests.cs (1)
38-48: Consider adding timeout handling for test robustness.The test awaits the second
ProcessNexttask (line 46) without an explicit timeout. If the lock release fails or the request hangs, the test could hang indefinitely.Consider adding a timeout:
using var successRequestResponse = await processNextTasks.First(x => x != conflictResponseTask); + // Or with timeout: + // using var successRequestResponse = await processNextTasks.First(x => x != conflictResponseTask).WaitAsync(TimeSpan.FromSeconds(30));This is optional—test hangs still indicate failure—but explicit timeouts can provide clearer diagnostics.
src/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs (4)
43-52: Add input validation for lock acquisition parameters.The method doesn't validate inputs, which could lead to confusing errors downstream:
expirationcould be negative or zeroinstanceGuidcould be emptyinstanceOwnerPartyIdcould be invalid (e.g., negative)Add validation at the start of the method:
public async Task<Guid> AcquireProcessLock(Guid instanceGuid, int instanceOwnerPartyId, TimeSpan expiration) { + ArgumentOutOfRangeException.ThrowIfNegativeOrZero(instanceOwnerPartyId, nameof(instanceOwnerPartyId)); + ArgumentOutOfRangeException.ThrowIfLessThanOrEqual(expiration, TimeSpan.Zero, nameof(expiration)); + if (instanceGuid == Guid.Empty) + throw new ArgumentException("Instance GUID cannot be empty.", nameof(instanceGuid)); + using var activity = _telemetry?.StartAcquireProcessLockActivity(instanceGuid, instanceOwnerPartyId);
52-52: TimeSpan to int conversion may truncate precision.Converting
expiration.TotalSecondstointtruncates sub-second precision and could overflow for very large durations (>24 days). For typical lock durations (seconds to minutes), this is unlikely to be an issue.If sub-second precision or durations >24 days are needed, consider using a long or validating the range.
68-71: Improve error specificity in JSON parsing exception handler.The generic catch logs the exception but then throws a different, less specific error. Consider including the parsing exception details in the thrown exception or rethrowing it directly.
catch (Exception e) { _logger.LogError(e, "Error reading response from the lock acquisition endpoint."); + throw new InvalidOperationException( + "Failed to parse lock acquisition response. See inner exception for details.", + e + ); } - -if (lockId is null || lockId.Value == Guid.Empty) -{ - throw PlatformHttpResponseSnapshotException.Create( - "The response from the lock acquisition endpoint was not expected.", - response - ); -}
43-82: Consider adding CancellationToken support.Both methods perform async HTTP operations but don't accept
CancellationTokenparameters. This prevents callers from canceling long-running lock operations and is inconsistent with other HTTP client methods in the codebase (per HttpClientExtension patterns).Based on coding guidelines, adding optional CancellationToken parameters is acceptable if documented in release notes.
-public async Task<Guid> AcquireProcessLock(Guid instanceGuid, int instanceOwnerPartyId, TimeSpan expiration) +public async Task<Guid> AcquireProcessLock(Guid instanceGuid, int instanceOwnerPartyId, TimeSpan expiration, CancellationToken cancellationToken = default) { // ... - using var response = await _client.PostAsync(token, apiUrl, content); + using var response = await _client.PostAsync(token, apiUrl, content, cancellationToken: cancellationToken); // ... - var lockResponse = await response.Content.ReadFromJsonAsync<ProcessLockResponse>(); + var lockResponse = await response.Content.ReadFromJsonAsync<ProcessLockResponse>(cancellationToken); } -public async Task ReleaseProcessLock(Guid instanceGuid, int instanceOwnerPartyId, Guid lockId) +public async Task ReleaseProcessLock(Guid instanceGuid, int instanceOwnerPartyId, Guid lockId, CancellationToken cancellationToken = default) { // ... - using var response = await _client.PatchAsync(token, apiUrl, content); + using var response = await _client.PatchAsync(token, apiUrl, content, cancellationToken: cancellationToken); }Also applies to: 84-102
src/Altinn.App.Api/Extensions/WebApplicationBuilderExtensions.cs (1)
31-31: Reconsider the middleware approach for a single endpoint.The
[EnableProcessLock]attribute is currently applied to only theNextElementendpoint inProcessController. Using middleware + attributes for a single endpoint introduces infrastructure overhead that direct client usage would avoid.For one endpoint, explicit lock management is clearer:
var lockId = await _processLockClient.AcquireProcessLock(...); try { // process logic } finally { await _processLockClient.ReleaseProcessLock(..., lockId); }This is more straightforward to understand, debug, and maintain. If additional endpoints require locking in the future, the middleware pattern can be introduced then without significant refactoring.
test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/process-lock/services/WaitForReleaseProcessTaskEnd.cs (1)
34-65: Endpoint wiring and DI registration look consistent with existing test-app patternsThe minimal endpoints for
/test/process-lock/release-waitand/test/process-lock/reset, plus DI registration viaServiceRegistration.RegisterServices, are straightforward and align well with the scenario-specific test app setup. No further changes needed here from my side.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Altinn.App.Api/Controllers/ProcessController.cs(2 hunks)src/Altinn.App.Api/Extensions/WebApplicationBuilderExtensions.cs(1 hunks)src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs(1 hunks)src/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs(1 hunks)src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLockRequest.cs(1 hunks)test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.CustomExpirationConfiguration_UsedInStorageApiCall.verified.txt(1 hunks)test/Altinn.App.Integration.Tests/ProcessLock/ProcessLockTests.cs(1 hunks)test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/process-lock/services/WaitForReleaseProcessTaskEnd.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLockRequest.cs
- test/Altinn.App.Api.Tests/Middleware/ProcessLockMiddlewareTests.CustomExpirationConfiguration_UsedInStorageApiCall.verified.txt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Api/Controllers/ProcessController.cstest/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/process-lock/services/WaitForReleaseProcessTaskEnd.cstest/Altinn.App.Integration.Tests/ProcessLock/ProcessLockTests.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cssrc/Altinn.App.Api/Extensions/WebApplicationBuilderExtensions.cs
**/test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/test/**/*.cs: Prefer xUnit asserts over FluentAssertions in tests
Mock external dependencies with Moq in tests
Files:
test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/process-lock/services/WaitForReleaseProcessTaskEnd.cstest/Altinn.App.Integration.Tests/ProcessLock/ProcessLockTests.cs
🧠 Learnings (14)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
src/Altinn.App.Api/Controllers/ProcessController.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cssrc/Altinn.App.Api/Extensions/WebApplicationBuilderExtensions.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
src/Altinn.App.Api/Controllers/ProcessController.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
src/Altinn.App.Api/Controllers/ProcessController.cstest/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/process-lock/services/WaitForReleaseProcessTaskEnd.cstest/Altinn.App.Integration.Tests/ProcessLock/ProcessLockTests.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which supports default interface methods.
Applied to files:
src/Altinn.App.Api/Controllers/ProcessController.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which fully supports default interface methods.
Applied to files:
src/Altinn.App.Api/Controllers/ProcessController.cs
📚 Learning: 2025-09-25T08:15:25.624Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Applied to files:
test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/process-lock/services/WaitForReleaseProcessTaskEnd.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Include integration tests for platform service interactions
Applied to files:
test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/process-lock/services/WaitForReleaseProcessTaskEnd.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/process-lock/services/WaitForReleaseProcessTaskEnd.cstest/Altinn.App.Integration.Tests/ProcessLock/ProcessLockTests.cs
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Integration.Tests/ProcessLock/ProcessLockTests.cs
📚 Learning: 2025-08-29T05:32:47.222Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/AppFixture.cs:958-958
Timestamp: 2025-08-29T05:32:47.222Z
Learning: In Altinn.App.Integration.Tests, the maintainer prefers fail-fast behavior (Environment.FailFast) when critical test infrastructure components like log reading fail, rather than graceful error handling, because diagnostics are essential for debugging test failures.
Applied to files:
test/Altinn.App.Integration.Tests/ProcessLock/ProcessLockTests.cs
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Integration.Tests/ProcessLock/ProcessLockTests.cs
📚 Learning: 2025-09-18T10:12:21.841Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1357
File: src/Altinn.App.Api/Infrastructure/Middleware/ScopeAuthorizationMiddleware.cs:289-293
Timestamp: 2025-09-18T10:12:21.841Z
Learning: In Altinn App security middleware like ScopeAuthorizationMiddleware, prefer explicit failures (throwing exceptions) over silently skipping unexpected endpoint types during discovery to prevent potential authorization gaps and ensure all endpoints are properly handled according to security configuration.
Applied to files:
src/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cssrc/Altinn.App.Api/Extensions/WebApplicationBuilderExtensions.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Remember to dispose IDisposable/IAsyncDisposable instances
Applied to files:
src/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs
🧬 Code graph analysis (1)
src/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs (4)
src/Altinn.App.Core/Extensions/HttpClientExtension.cs (6)
Task(20-43)Task(55-78)Task(89-110)Task(124-145)Task(157-180)Task(191-212)src/Altinn.App.Core/Helpers/PlatformHttpResponseSnapshotException.cs (5)
Task(65-96)Task(207-275)PlatformHttpResponseSnapshotException(18-322)PlatformHttpResponseSnapshotException(108-166)PlatformHttpResponseSnapshotException(179-197)src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLockRequest.cs (1)
ProcessLockRequest(3-6)src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLockResponse.cs (1)
ProcessLockResponse(3-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Static code analysis
- GitHub Check: Analyze (csharp)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
🔇 Additional comments (7)
test/Altinn.App.Integration.Tests/ProcessLock/ProcessLockTests.cs (2)
7-10: LGTM! Clean test class structure.The class follows best practices: sealed modifier, primary constructor syntax, and proper xUnit fixture pattern for integration tests.
11-49: Verify the middleware approach aligns with architectural preferences.The test logic is well-designed and correctly validates the concurrent process-lock scenario:
- Concurrent requests are issued
- One gets 409 Conflict (completes immediately)
- Lock is released via test endpoint
- The other completes successfully
However, in the PR comments, reviewer ivarne questioned introducing middleware for what may be only a few endpoints, suggesting instead to use
ProcessLockClientdirectly in controller methods withusingor try/finally blocks. Please confirm this middleware-based approach aligns with the team's architectural decision before expanding test coverage.src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs (1)
117-117: LGTM: HTTP client registration follows established pattern.The ProcessLockClient registration is consistent with other storage client registrations in this method. The concrete-type registration (without interface) is appropriate given that ProcessLockClient is internal sealed.
src/Altinn.App.Api/Controllers/ProcessController.cs (1)
257-257: EnableProcessLock attribute applied to critical process advancement endpoint.The attribute application is appropriate for NextElement, which is the primary endpoint for advancing process state. This endpoint would benefit from exclusive locking to prevent concurrent process modifications.
Note: If the team decides to move away from the middleware approach (per ivarne's suggestion), this would become a direct call to ProcessLockClient with try/finally for cleanup.
src/Altinn.App.Core/Infrastructure/Clients/Storage/ProcessLockClient.cs (2)
43-82: Resource disposal correctly implemented.The
using var responseat line 55 ensures proper disposal in all code paths. The past review concerns about resource leaks have been addressed:
- Line 59:
CreateAndDisposeHttpResponsehandles disposal on error- Lines 75-78:
Createreceives the response while it's still in the using block; the Create method copies response data, then the using disposes the original
84-102: Release method implementation looks correct.The ReleaseProcessLock method properly disposes the response and handles errors via CreateAndDisposeHttpResponse. Setting TTL to 0 to release the lock is a clear approach.
test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/process-lock/services/WaitForReleaseProcessTaskEnd.cs (1)
15-31: No changes needed — code is correct as-is
TaskCompletionSource(non-generic form, added in .NET 5) is the correct choice here. The non-generic variant returnsTaskand its parameterlessTrySetResult()method correctly completes the task. The code compiles and runs successfully.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs (1)
46-46: Consider the reviewer's alternative approach.The PR reviewer ivarne suggests using ProcessLockClient directly in controller methods with a
usingstatement for tighter lock scope control. The current implementation acquires the lock in ProcessEngine and holds it until request completion (via scoped disposal). This means the lock is held during error handling, response serialization, and any other operations after ProcessNext returns.Consider whether the lock should be held for the entire request duration or just the minimal critical section around process state changes.
Also applies to: 79-79
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/Altinn.App.Api/Controllers/ProcessController.cs(1 hunks)src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs(3 hunks)src/Altinn.App.Core/Internal/Process/ProcessEngine.cs(4 hunks)src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLockOptions.cs(1 hunks)src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLocker.cs(1 hunks)test/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.CustomExpirationConfiguration_UsedInStorageApiCall.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.EmptyJsonResponseBody_ThrowsPlatformHttpException.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.InvalidInstanceId_ThrowsArgumentException.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.NullResponseBody_ThrowsPlatformHttpException.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=Conflict.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=InternalServerError.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=NotFound.verified.txt(1 hunks)test/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.cs(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- test/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=InternalServerError.verified.txt
- test/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.InvalidInstanceId_ThrowsArgumentException.verified.txt
- test/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=NotFound.verified.txt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.cstest/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLockOptions.cssrc/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLocker.cssrc/Altinn.App.Api/Controllers/ProcessController.cs
**/test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/test/**/*.cs: Prefer xUnit asserts over FluentAssertions in tests
Mock external dependencies with Moq in tests
Files:
test/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.cs
🧠 Learnings (13)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.NullResponseBody_ThrowsPlatformHttpException.verified.txttest/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.CustomExpirationConfiguration_UsedInStorageApiCall.verified.txttest/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=Conflict.verified.txttest/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.cstest/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.EmptyJsonResponseBody_ThrowsPlatformHttpException.verified.txt
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.NullResponseBody_ThrowsPlatformHttpException.verified.txttest/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.CustomExpirationConfiguration_UsedInStorageApiCall.verified.txttest/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=Conflict.verified.txttest/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.EmptyJsonResponseBody_ThrowsPlatformHttpException.verified.txt
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Api/Controllers/ProcessController.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.cstest/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.cssrc/Altinn.App.Api/Controllers/ProcessController.cs
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.cssrc/Altinn.App.Api/Controllers/ProcessController.cs
📚 Learning: 2025-09-21T09:04:43.977Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: src/Altinn.App.Core/Features/IInstanceDataAccessor.cs:36-41
Timestamp: 2025-09-21T09:04:43.977Z
Learning: The IInstanceDataAccessor interface in Altinn.App.Core is designed for consumption only - users are not expected to implement this interface themselves, only use framework-provided implementations.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which supports default interface methods.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which fully supports default interface methods.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs
📚 Learning: 2025-09-07T20:03:48.030Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1463
File: test/Altinn.App.SourceGenerator.Tests/DiagnosticTests.RunJsonError.verified.txt:1-21
Timestamp: 2025-09-07T20:03:48.030Z
Learning: In Altinn.App.SourceGenerator.Tests, the path C:\temp\config\applicationmetadata.json is a hardcoded test fixture path required by Roslyn and is used consistently across all operating systems, not an actual filesystem path that varies by OS.
Applied to files:
test/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=Conflict.verified.txttest/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
test/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/test/**/*.cs : Mock external dependencies with Moq in tests
Applied to files:
test/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Register services in DI container properly following existing patterns
Applied to files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
🧬 Code graph analysis (4)
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs (1)
src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLocker.cs (1)
ProcessLocker(8-89)
test/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.cs (4)
src/Altinn.App.Core/Configuration/PlatformSettings.cs (1)
PlatformSettings(7-74)src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLocker.cs (1)
ProcessLocker(8-89)src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLockResponse.cs (1)
ProcessLockResponse(3-6)src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLockOptions.cs (1)
ProcessLockOptions(3-6)
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs (1)
src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLocker.cs (1)
ProcessLocker(8-89)
src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLocker.cs (3)
src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLockOptions.cs (1)
ProcessLockOptions(3-6)src/Altinn.App.Core/Features/Signing/Services/SigningDelegationService.cs (1)
Guid(146-156)src/Altinn.App.Core/Models/InstanceIdentifier.cs (1)
InstanceOwnerPartyId(111-118)
🪛 GitHub Actions: Build and Test on windows, macos and ubuntu
src/Altinn.App.Api/Controllers/ProcessController.cs
[error] 12-12: IDE0005: Using directive is unnecessary. (dotnet build step failed: 'dotnet build solutions/All.sln -v m')
🪛 GitHub Check: Run dotnet build and test (macos-latest)
src/Altinn.App.Api/Controllers/ProcessController.cs
[failure] 12-12:
Using directive is unnecessary. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0005)
[failure] 12-12:
Using directive is unnecessary. (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0005)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (csharp)
- GitHub Check: Static code analysis
🔇 Additional comments (16)
src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLockOptions.cs (1)
1-6: LGTM!The options class follows coding guidelines (internal, sealed) and provides a sensible default expiration of 5 minutes.
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs (2)
118-118: LGTM!ProcessLockClient registration as an HTTP client follows the established pattern.
369-369: LGTM!ProcessLocker is correctly registered as scoped, which ensures it will be disposed at the end of the request, releasing any acquired locks via its IAsyncDisposable implementation.
src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLocker.cs (4)
8-18: LGTM!The primary constructor pattern and field initializers are clean. The null check for HttpContext ensures the ProcessLocker is always constructed with a valid context, which is appropriate for its scoped lifetime.
20-35: LGTM!The idempotent locking logic is correct, and the flow properly handles lock acquisition and storage.
37-52: LGTM!The route value extraction logic safely handles parsing with appropriate null checks and TryParse methods.
54-74: LGTM!The dispose implementation correctly follows the pattern of not throwing exceptions from disposal. The try-catch ensures graceful handling of release failures while still logging errors.
test/Altinn.App.Core.Tests/Internal/Process/ProcessLockTests.cs (9)
17-83: LGTM!The fixture pattern is well-structured with proper setup, teardown, and helper methods for building WireMock request matchers.
85-136: LGTM!Comprehensive happy path test that verifies the complete lock acquisition, operation, and release cycle with proper request verification.
138-191: LGTM!Excellent test for verifying idempotent behavior - ensures multiple
LockAsynccalls only acquire the lock once.
193-223: LGTM!Critical test ensuring the dispose pattern properly releases locks even when exceptions occur, preventing resource leaks.
225-261: LGTM!Well-designed test verifying that custom expiration configuration is properly propagated to the storage API call.
263-291: LGTM!Important test confirming that disposal doesn't throw even when lock release fails, which aligns with IAsyncDisposable best practices.
293-317: LGTM!Comprehensive theory test covering multiple HTTP error status codes and verifying proper exception handling with snapshot testing.
319-344: LGTM!Good edge case test for handling null response bodies despite successful HTTP status.
346-371: LGTM!Important edge case test for handling empty JSON objects where required fields are missing.
| await _processLocker.LockAsync(); | ||
|
|
There was a problem hiding this comment.
Lock acquisition lacks cancellation support.
The lock acquisition at Line 258 does not pass the CancellationToken ct parameter, so if the request is cancelled after authorization but before or during lock acquisition, the cancellation won't be honored. This could result in acquiring a lock for a cancelled request.
Consider adding CancellationToken support to ProcessLocker.LockAsync and ProcessLockClient.AcquireProcessLock to allow graceful cancellation.
🤖 Prompt for AI Agents
In src/Altinn.App.Core/Internal/Process/ProcessEngine.cs around lines 258-259,
the call await _processLocker.LockAsync() does not pass the CancellationToken ct
so lock acquisition cannot be cancelled; modify ProcessLocker.LockAsync to
accept a CancellationToken parameter and propagate it into its implementation,
update ProcessLockClient.AcquireProcessLock (and any underlying HTTP or RPC
calls) to accept and forward the token as well, and then call await
_processLocker.LockAsync(ct) here; also update all other callers and interface
signatures accordingly to preserve cancellation support end-to-end.
| [LoggerMessage(1, LogLevel.Debug, "Failed to acquire process lock.")] | ||
| private static partial void LogLockAcquisitionFailed(ILogger logger); |
There was a problem hiding this comment.
Remove unused logger method.
LogLockAcquisitionFailed is defined but never invoked in the code. Since LockAsync allows exceptions to propagate from client.AcquireProcessLock, there's no code path that would call this logger method.
- [LoggerMessage(1, LogLevel.Debug, "Failed to acquire process lock.")]
- private static partial void LogLockAcquisitionFailed(ILogger logger);
-
- [LoggerMessage(2, LogLevel.Debug, "Acquired process lock with id: {LockId}")]
+ [LoggerMessage(1, LogLevel.Debug, "Acquired process lock with id: {LockId}")]
private static partial void LogLockAcquired(ILogger logger, Guid lockId);
- [LoggerMessage(3, LogLevel.Debug, "Released process lock with id: {LockId}")]
+ [LoggerMessage(2, LogLevel.Debug, "Released process lock with id: {LockId}")]
private static partial void LogLockReleased(ILogger logger, Guid lockId);
- [LoggerMessage(4, LogLevel.Error, "Failed to release process lock with id: {LockId}")]
+ [LoggerMessage(3, LogLevel.Error, "Failed to release process lock with id: {LockId}")]
private static partial void LogLockReleaseFailed(ILogger logger, Guid lockId, Exception e);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [LoggerMessage(1, LogLevel.Debug, "Failed to acquire process lock.")] | |
| private static partial void LogLockAcquisitionFailed(ILogger logger); | |
| [LoggerMessage(1, LogLevel.Debug, "Acquired process lock with id: {LockId}")] | |
| private static partial void LogLockAcquired(ILogger logger, Guid lockId); | |
| [LoggerMessage(2, LogLevel.Debug, "Released process lock with id: {LockId}")] | |
| private static partial void LogLockReleased(ILogger logger, Guid lockId); | |
| [LoggerMessage(3, LogLevel.Error, "Failed to release process lock with id: {LockId}")] | |
| private static partial void LogLockReleaseFailed(ILogger logger, Guid lockId, Exception e); |
🤖 Prompt for AI Agents
In src/Altinn.App.Core/Internal/Process/ProcessLock/ProcessLocker.cs around
lines 78-79, the static partial logger method LogLockAcquisitionFailed is
defined with LoggerMessage but is never called; remove the unused method and its
LoggerMessage attribute to avoid dead code and unused generated logger members,
and run build to ensure no references remain (if there are future needs,
reintroduce a logger call where AcquireProcessLock exceptions are caught instead
of letting them propagate).
test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs (1)
258-259: Lock acquisition does not propagateCancellationToken.The
CancellationToken ctparameter is available but not passed toLockAsync(). If the request is cancelled during lock acquisition, the cancellation won't be honored. Consider addingCancellationTokensupport toInstanceLocker.LockAsync.This was flagged in a previous review. Consider updating
InstanceLocker.LockAsyncto accept and propagate the cancellation token.test/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.cs (1)
375-396: Fix test implementation and naming.This test has two issues that were flagged in a previous review:
Critical bug in test setup: Line 383 creates a new empty
DefaultHttpContextinstead of using the one configured with invalid route values on lines 380-382. The test validates behavior with an empty HttpContext rather than invalid instance identifiers.Test name mismatch: The test name indicates it expects
ArgumentException, but line 387 assertsInvalidOperationException.🔎 Proposed fix
[Fact] - public async Task InvalidInstanceId_ThrowsArgumentException() + public async Task InvalidInstanceId_ThrowsInvalidOperationException() { using var fixture = Fixture.Create(services => { var httpContext = new DefaultHttpContext(); httpContext.Request.RouteValues.Add("instanceOwnerPartyId", "invalid"); httpContext.Request.RouteValues.Add("instanceGuid", "format"); - var httpContextAccessor = new HttpContextAccessor { HttpContext = new DefaultHttpContext() }; + var httpContextAccessor = new HttpContextAccessor { HttpContext = httpContext }; services.AddSingleton<IHttpContextAccessor>(httpContextAccessor); });
🧹 Nitpick comments (3)
test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/instance-lock/services/WaitForReleaseProcessTaskEnd.cs (1)
13-32: Thread-safety concern in static synchronization primitives.The
Reset()method has a potential race condition: betweenTrySetResult()and creating a newTaskCompletionSource, another caller could invokeRelease()orReset()on the old instance. While this is test scaffold code with controlled execution, consider usingInterlocked.Exchangefor atomicity.🔎 Suggested improvement for thread-safety
public sealed class WaitForReleaseProcessTaskEnd : IProcessTaskEnd { - private static TaskCompletionSource _signal = new(); + private static TaskCompletionSource _signal = new(TaskCreationOptions.RunContinuationsAsynchronously); public Task End(string taskId, Instance instance) { return _signal.Task; } public static void Release() { _signal.TrySetResult(); } public static void Reset() { - _signal.TrySetResult(); - _signal = new TaskCompletionSource(); + var oldSignal = Interlocked.Exchange(ref _signal, new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously)); + oldSignal.TrySetResult(); } }Note:
TaskCreationOptions.RunContinuationsAsynchronouslyprevents inline continuation execution which can cause issues in tests.src/Altinn.App.Core/Internal/Process/ProcessEngine.cs (1)
79-79: ResolveInstanceLockervia constructor injection instead of service locator.Currently
InstanceLockeris resolved viaGetRequiredServicewhile other dependencies are constructor-injected. SinceInstanceLockeris scoped andProcessEngineappears to be scoped or transient, direct constructor injection would be more consistent and testable.🔎 Suggested refactor
public ProcessEngine( IProcessReader processReader, IProcessNavigator processNavigator, IProcessEventHandlerDelegator processEventsDelegator, IProcessEventDispatcher processEventDispatcher, UserActionService userActionService, IAuthenticationContext authenticationContext, IServiceProvider serviceProvider, IProcessEngineAuthorizer processEngineAuthorizer, IValidationService validationService, IInstanceClient instanceClient, ILogger<ProcessEngine> logger, + InstanceLocker instanceLocker, Telemetry? telemetry = null ) { // ... existing assignments ... - _instanceLocker = serviceProvider.GetRequiredService<InstanceLocker>(); + _instanceLocker = instanceLocker; }src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs (1)
62-79: Consider preserving the original exception for debugging.The generic catch clause logs the error but then throws a different exception without the original cause. This makes debugging harder when the JSON deserialization fails.
🔎 Proposed improvement
string? lockToken = null; + Exception? deserializationException = null; try { var lockResponse = await response.Content.ReadFromJsonAsync<InstanceLockResponse>(); lockToken = lockResponse?.LockToken; } - catch (Exception e) + catch (JsonException e) { _logger.LogError(e, "Error reading response from the lock acquisition endpoint."); + deserializationException = e; } if (string.IsNullOrEmpty(lockToken)) { - throw PlatformHttpResponseSnapshotException.Create( + throw PlatformHttpResponseSnapshotException.CreateWithInnerException( "The response from the lock acquisition endpoint was not expected.", - response + response, + deserializationException ); }This addresses the static analysis warning about the generic catch clause while preserving diagnostic information.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
Directory.Packages.propssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Features/Telemetry/Telemetry.InstanceLockClient.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cssrc/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cssrc/Altinn.App.Core/Internal/Process/ProcessEngine.cstest/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.CustomTtl_UsedInStorageApiCall.verified.txttest/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.EmptyJsonResponseBody_ThrowsPlatformHttpException.verified.txttest/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.InvalidInstanceId_ThrowsArgumentException.verified.txttest/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.NullResponseBody_ThrowsPlatformHttpException.verified.txttest/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=Conflict.verified.txttest/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=InternalServerError.verified.txttest/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=NotFound.verified.txttest/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.cstest/Altinn.App.Integration.Tests/InstanceLocking/InstanceLockTests.cstest/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/instance-lock/services/WaitForReleaseProcessTaskEnd.cs
✅ Files skipped from review due to trivial changes (3)
- test/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=NotFound.verified.txt
- test/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.CustomTtl_UsedInStorageApiCall.verified.txt
- test/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.EmptyJsonResponseBody_ThrowsPlatformHttpException.verified.txt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.cstest/Altinn.App.Integration.Tests/InstanceLocking/InstanceLockTests.cstest/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.cssrc/Altinn.App.Core/Features/Telemetry/Telemetry.InstanceLockClient.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cstest/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/instance-lock/services/WaitForReleaseProcessTaskEnd.cssrc/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs
**/test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/test/**/*.cs: Prefer xUnit asserts over FluentAssertions in tests
Mock external dependencies with Moq in tests
Files:
test/Altinn.App.Integration.Tests/InstanceLocking/InstanceLockTests.cstest/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.cstest/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/instance-lock/services/WaitForReleaseProcessTaskEnd.cs
🧠 Learnings (18)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
Directory.Packages.propssrc/Altinn.App.Core/Internal/Process/ProcessEngine.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which supports default interface methods.
Applied to files:
Directory.Packages.propssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which fully supports default interface methods.
Applied to files:
Directory.Packages.propssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=Conflict.verified.txttest/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.NullResponseBody_ThrowsPlatformHttpException.verified.txttest/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.InvalidInstanceId_ThrowsArgumentException.verified.txttest/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=InternalServerError.verified.txt
📚 Learning: 2025-09-07T20:03:48.030Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1463
File: test/Altinn.App.SourceGenerator.Tests/DiagnosticTests.RunJsonError.verified.txt:1-21
Timestamp: 2025-09-07T20:03:48.030Z
Learning: In Altinn.App.SourceGenerator.Tests, the path C:\temp\config\applicationmetadata.json is a hardcoded test fixture path required by Roslyn and is used consistently across all operating systems, not an actual filesystem path that varies by OS.
Applied to files:
test/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=Conflict.verified.txttest/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.NullResponseBody_ThrowsPlatformHttpException.verified.txttest/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=InternalServerError.verified.txt
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=Conflict.verified.txttest/Altinn.App.Integration.Tests/InstanceLocking/InstanceLockTests.cstest/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.NullResponseBody_ThrowsPlatformHttpException.verified.txttest/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=InternalServerError.verified.txt
📚 Learning: 2025-09-21T09:04:43.977Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: src/Altinn.App.Core/Features/IInstanceDataAccessor.cs:36-41
Timestamp: 2025-09-21T09:04:43.977Z
Learning: The IInstanceDataAccessor interface in Altinn.App.Core is designed for consumption only - users are not expected to implement this interface themselves, only use framework-provided implementations.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.cstest/Altinn.App.Integration.Tests/InstanceLocking/InstanceLockTests.cstest/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.cstest/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.InvalidInstanceId_ThrowsArgumentException.verified.txtsrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cstest/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/instance-lock/services/WaitForReleaseProcessTaskEnd.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cstest/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/instance-lock/services/WaitForReleaseProcessTaskEnd.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-08-29T05:32:47.222Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/AppFixture.cs:958-958
Timestamp: 2025-08-29T05:32:47.222Z
Learning: In Altinn.App.Integration.Tests, the maintainer prefers fail-fast behavior (Environment.FailFast) when critical test infrastructure components like log reading fail, rather than graceful error handling, because diagnostics are essential for debugging test failures.
Applied to files:
test/Altinn.App.Integration.Tests/InstanceLocking/InstanceLockTests.cstest/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.InvalidInstanceId_ThrowsArgumentException.verified.txttest/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=InternalServerError.verified.txt
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Follow the AppFixture pattern for integration tests: Central orchestrator managing test lifecycle with feature-specific operations
Applied to files:
test/Altinn.App.Integration.Tests/InstanceLocking/InstanceLockTests.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Register services in DI container properly following existing patterns
Applied to files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-09-25T08:15:25.624Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Applied to files:
test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/instance-lock/services/WaitForReleaseProcessTaskEnd.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Include integration tests for platform service interactions
Applied to files:
test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/instance-lock/services/WaitForReleaseProcessTaskEnd.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Remember to dispose IDisposable/IAsyncDisposable instances
Applied to files:
src/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs
📚 Learning: 2025-09-18T10:12:21.841Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1357
File: src/Altinn.App.Api/Infrastructure/Middleware/ScopeAuthorizationMiddleware.cs:289-293
Timestamp: 2025-09-18T10:12:21.841Z
Learning: In Altinn App security middleware like ScopeAuthorizationMiddleware, prefer explicit failures (throwing exceptions) over silently skipping unexpected endpoint types during discovery to prevent potential authorization gaps and ensure all endpoints are properly handled according to security configuration.
Applied to files:
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs
🧬 Code graph analysis (6)
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs (1)
src/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs (1)
InstanceLocker(7-89)
test/Altinn.App.Integration.Tests/InstanceLocking/InstanceLockTests.cs (2)
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs (2)
Task(43-82)Task(84-97)src/Altinn.App.Api/Models/InstansiationInstance.cs (1)
InstansiationInstance(10-42)
test/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.cs (3)
src/Altinn.App.Core/Configuration/PlatformSettings.cs (1)
PlatformSettings(7-74)src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs (4)
InstanceLockClient(16-98)InstanceLockClient(24-41)Task(43-82)Task(84-97)src/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs (1)
InstanceLocker(7-89)
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs (2)
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs (2)
InstanceLockClient(16-98)InstanceLockClient(24-41)src/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs (1)
InstanceLocker(7-89)
src/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs (3)
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs (2)
InstanceLockClient(16-98)InstanceLockClient(24-41)src/Altinn.App.Core/Features/Signing/Services/SigningDelegationService.cs (1)
Guid(146-156)src/Altinn.App.Core/Models/InstanceIdentifier.cs (1)
InstanceOwnerPartyId(111-118)
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs (3)
src/Altinn.App.Core/Features/Telemetry/Telemetry.InstanceLockClient.cs (1)
Telemetry(5-24)src/Altinn.App.Core/Configuration/PlatformSettings.cs (1)
PlatformSettings(7-74)src/Altinn.App.Core/Constants/General.cs (1)
General(6-42)
🔇 Additional comments (9)
test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/instance-lock/services/WaitForReleaseProcessTaskEnd.cs (1)
58-65: LGTM!The service registration follows established DI patterns. Transient for
IProcessTaskEndand singleton forIEndpointConfiguratoralign with their usage semantics.src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs (2)
118-118: LGTM!The
InstanceLockClientHTTP client registration follows the established pattern used by other platform clients inAddPlatformServices.
369-370: LGTM!Scoped lifetime for
InstanceLockeris correct since it maintains per-request lock state viaHttpContextand implementsIAsyncDisposablefor lock release. The registration withinAddProcessServicesappropriately groups it with other process-related services.src/Altinn.App.Core/Internal/Process/ProcessEngine.cs (1)
258-259: Consider lock scope for error scenarios.The lock is acquired before validation and service task execution. If validation fails (lines 340-360) or a service task fails, the lock remains held until request completion when
InstanceLocker.DisposeAsyncis called by the DI container. This is likely acceptable since:
- Failed requests complete quickly
- Lock TTL provides a safety net (5 minutes default)
However, verify this aligns with the intended behavior for concurrent request handling.
src/Altinn.App.Core/Features/Telemetry/Telemetry.InstanceLockClient.cs (1)
1-24: LGTM!The telemetry helpers follow the established patterns in the codebase. Internal accessibility is correct per coding guidelines, and the null-conditional operators properly handle cases where activity creation fails.
test/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.cs (1)
16-66: LGTM - Well-structured test fixture.The fixture properly implements
IDisposableand cleans up both the WireMock server and ServiceProvider. The helper methods for building request matchers are clean and reusable.src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs (1)
16-41: LGTM - Well-structured HTTP client.The client follows the typed HttpClient pattern correctly. Internal sealed class per guidelines, proper header configuration, and nullable Telemetry dependency handled correctly.
src/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs (2)
7-55: LGTM - Clean implementation of the instance locker.The implementation correctly follows the coding guidelines:
- Internal sealed partial class
- Implements IAsyncDisposable properly
- Idempotent lock acquisition (early return if already locked)
- Uses source-generated logging for performance
57-77: LGTM - Proper dispose pattern.The
DisposeAsyncimplementation correctly:
- Handles the case where no lock was acquired
- Catches and logs exceptions from release without re-throwing (appropriate for dispose)
- Intentionally keeps
_locknon-null on failure to prevent double-release attempts
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.cs (1)
18-84: Excellent test fixture design with comprehensive infrastructure setup.The Fixture class properly sets up WireMock, configures DI, and provides helper methods for request matching. Good use of disposal patterns and proper service registration.
Optional: Consider generating unique GUID per fixture instance
The static
_instanceGuidfield (line 20) is shared across all test fixture instances. While this doesn't cause functional issues since each test creates its own WireMock server, generating a unique GUID per fixture would improve test isolation semantics.- private static readonly Guid _instanceGuid = Guid.NewGuid(); + private readonly Guid _instanceGuid = Guid.NewGuid();This ensures each test fixture has fully independent instance identifiers.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.InvalidInstanceId_ThrowsInvalidOperationException.verified.txttest/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.cs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
test/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.cs
**/test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/test/**/*.cs: Prefer xUnit asserts over FluentAssertions in tests
Mock external dependencies with Moq in tests
Files:
test/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.cs
🧠 Learnings (4)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
test/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.cstest/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.InvalidInstanceId_ThrowsInvalidOperationException.verified.txt
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.InvalidInstanceId_ThrowsInvalidOperationException.verified.txt
📚 Learning: 2025-08-29T05:32:47.222Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/AppFixture.cs:958-958
Timestamp: 2025-08-29T05:32:47.222Z
Learning: In Altinn.App.Integration.Tests, the maintainer prefers fail-fast behavior (Environment.FailFast) when critical test infrastructure components like log reading fail, rather than graceful error handling, because diagnostics are essential for debugging test failures.
Applied to files:
test/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.InvalidInstanceId_ThrowsInvalidOperationException.verified.txt
🧬 Code graph analysis (1)
test/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.cs (3)
src/Altinn.App.Core/Configuration/PlatformSettings.cs (1)
PlatformSettings(7-74)src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs (4)
InstanceLockClient(16-98)InstanceLockClient(24-41)Task(43-82)Task(84-97)src/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs (1)
InstanceLocker(7-89)
🔇 Additional comments (2)
test/Altinn.App.Core.Tests/Internal/Process/InstanceLockTests.cs (2)
86-373: Comprehensive test coverage across happy paths, error scenarios, and edge cases.The test suite thoroughly validates:
- Lock acquisition and release sequencing with perfect-match verification
- Idempotent multiple lock calls
- Lock release on exceptions
- Custom TTL propagation to storage API
- Graceful handling of release failures
- Platform exception mapping for various HTTP status codes (409, 404, 500)
- Edge cases for null and empty response bodies
Good use of xUnit assertions (per coding guidelines), proper async/disposal patterns, and Verify for snapshot testing.
375-396: Correctly implements the invalid instance ID test.The test properly configures the HttpContext with invalid route values and correctly assigns it to the HttpContextAccessor, verifying that
InvalidOperationExceptionis thrown when instance identifiers cannot be extracted. This addresses the issue noted in past review comments.
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs
Fixed
Show fixed
Hide fixed
| catch (Exception e) | ||
| { | ||
| LogLockReleaseFailed(logger, _lock.InstanceGuid, e); | ||
| return; | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 days ago
In general, to fix a generic catch clause, we:
- Identify the exception types that are expected, operational, and safe to handle locally.
- Catch those specific exception types and perform the current behavior (logging, swallowing, etc.).
- Optionally add a final
catchthat logs and rethrows, or remove it, so truly unexpected or critical exceptions are not silently swallowed.
For this code, we want to:
- Keep the current behavior (log an error and not throw) for expected errors when releasing the lock (e.g., the lock no longer exists, network/client issues).
- Avoid swallowing everything, particularly
OperationCanceledException/TaskCanceledException(to respect cancellation) and serious runtime failures.
A minimal, non-breaking change within the shown snippet is:
- Replace
catch (Exception e)with:- One or more specific catches for expected exceptions from
ReleaseInstanceLock(e.g.,TaskCanceledException/OperationCanceledException) with appropriate handling. - A final
catch (Exception e)that logs and rethrows or just rethrows, but that would change behavior. To avoid changing public behavior too much, we can:- Specifically catch cancellation exceptions and rethrow them (so cancellation is respected).
- Keep a generic catch for the remaining exceptions but narrow it by excluding
OperationCanceledException/TaskCanceledExceptionusing awhenfilter, which static analyzers treat as less severe because cancellation is no longer swallowed.
- One or more specific catches for expected exceptions from
Since we do not see the implementation of InstanceLockClient.ReleaseInstanceLock, we should avoid assuming custom exception types. We can:
- Add
using System.Threading;if needed, but here we can referenceOperationCanceledException(inSystem) andTaskCanceledException(inSystem.Threading.Tasks) without new usings because they’re inmscorlib/System.Private.CoreLiband typically available; no extra packages are required. - Update the
DisposeAsynccatch to:
try
{
await client.ReleaseInstanceLock(_lock.InstanceGuid, _lock.InstanceOwnerPartyId, _lock.LockToken);
}
catch (OperationCanceledException)
{
throw;
}
catch (TaskCanceledException)
{
throw;
}
catch (Exception e)
{
LogLockReleaseFailed(logger, _lock.InstanceGuid, e);
return;
}This keeps current behavior for most errors but ensures cancellation is not swallowed. It narrows the effective generic catch and addresses the core CodeQL concern without changing external behavior in the typical (non-cancellation) case.
All changes are confined to DisposeAsync in src/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs; no new methods or external libraries are needed.
| @@ -65,6 +65,16 @@ | ||
| { | ||
| await client.ReleaseInstanceLock(_lock.InstanceGuid, _lock.InstanceOwnerPartyId, _lock.LockToken); | ||
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| // Preserve cancellation semantics by not swallowing cancellation-related exceptions. | ||
| throw; | ||
| } | ||
| catch (TaskCanceledException) | ||
| { | ||
| // Preserve cancellation semantics by not swallowing cancellation-related exceptions. | ||
| throw; | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| LogLockReleaseFailed(logger, _lock.InstanceGuid, e); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs (1)
11-11: Unused import.
Microsoft.AspNetCore.Httpdoes not appear to be used in this file. Consider removing it.🔎 Proposed fix
-using Microsoft.AspNetCore.Http;test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cs (1)
63-63: Consider service lifetime alignment with production.
InstanceLockeris registered asTransienthere, but in production it's likelyScoped(per-request) since it tracks lock state. For these tests with explicitCreateAsyncScope()usage,Transientworks correctly, but consider usingAddScopedto better mirror production behavior.🔎 Proposed fix
- services.AddTransient<InstanceLocker, InstanceLocker>(); + services.AddScoped<InstanceLocker>();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cstest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.CustomTtl_UsedInStorageApiCall.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.EmptyJsonResponseBody_ThrowsPlatformHttpException.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.InvalidInstanceId_ThrowsInvalidOperationException.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.NullResponseBody_ThrowsPlatformHttpException.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=Conflict.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=InternalServerError.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=NotFound.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cs
✅ Files skipped from review due to trivial changes (5)
- test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=Conflict.verified.txt
- test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=NotFound.verified.txt
- test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.NullResponseBody_ThrowsPlatformHttpException.verified.txt
- test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=InternalServerError.verified.txt
- test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.InvalidInstanceId_ThrowsInvalidOperationException.verified.txt
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs
**/test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/test/**/*.cs: Prefer xUnit asserts over FluentAssertions in tests
Mock external dependencies with Moq in tests
Files:
test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cs
🧠 Learnings (8)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.CustomTtl_UsedInStorageApiCall.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.EmptyJsonResponseBody_ThrowsPlatformHttpException.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cs
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.CustomTtl_UsedInStorageApiCall.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.EmptyJsonResponseBody_ThrowsPlatformHttpException.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cs
📚 Learning: 2025-09-07T20:03:48.030Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1463
File: test/Altinn.App.SourceGenerator.Tests/DiagnosticTests.RunJsonError.verified.txt:1-21
Timestamp: 2025-09-07T20:03:48.030Z
Learning: In Altinn.App.SourceGenerator.Tests, the path C:\temp\config\applicationmetadata.json is a hardcoded test fixture path required by Roslyn and is used consistently across all operating systems, not an actual filesystem path that varies by OS.
Applied to files:
test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.CustomTtl_UsedInStorageApiCall.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.EmptyJsonResponseBody_ThrowsPlatformHttpException.verified.txt
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cs
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs
📚 Learning: 2025-09-18T10:12:21.841Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1357
File: src/Altinn.App.Api/Infrastructure/Middleware/ScopeAuthorizationMiddleware.cs:289-293
Timestamp: 2025-09-18T10:12:21.841Z
Learning: In Altinn App security middleware like ScopeAuthorizationMiddleware, prefer explicit failures (throwing exceptions) over silently skipping unexpected endpoint types during discovery to prevent potential authorization gaps and ensure all endpoints are properly handled according to security configuration.
Applied to files:
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs
🧬 Code graph analysis (2)
test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cs (3)
src/Altinn.App.Core/Configuration/PlatformSettings.cs (1)
PlatformSettings(7-74)src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs (4)
InstanceLockClient(17-112)InstanceLockClient(26-41)Task(43-91)Task(93-111)src/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs (1)
InstanceLocker(7-89)
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs (3)
src/Altinn.App.Core/Features/Telemetry/Telemetry.InstanceLockClient.cs (1)
Telemetry(5-24)src/Altinn.App.Core/Configuration/PlatformSettings.cs (1)
PlatformSettings(7-74)src/Altinn.App.Core/Constants/General.cs (1)
General(6-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Static code analysis
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (16)
test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.EmptyJsonResponseBody_ThrowsPlatformHttpException.verified.txt (1)
1-32: Verified snapshot file – skipping review.This is a *.verified.txt snapshot file used by the Verify library to capture test expectations. Per established guidance in the codebase, these verification snapshots should not be reviewed for code style or logic; they are auto-generated test artifacts.
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs (4)
26-41: LGTM!Constructor follows the standard typed HttpClient pattern for DI registration. The nullable
Telemetryparameter with default value allows graceful degradation when telemetry is not configured.
59-59: Consider TimeSpan precision loss.Casting
expiration.TotalSecondstointtruncates any fractional seconds. This is acceptable for lock TTLs which are typically whole seconds, but worth noting if sub-second precision becomes relevant.
69-90: LGTM!The exception handling is appropriately scoped—catching only
JsonExceptionandInvalidOperationExceptionfor deserialization failures. The fallback to throwPlatformHttpResponseSnapshotExceptionwith a descriptive message when the lock token is missing provides good error context.
93-111: LGTM!The release implementation is clean and follows the same patterns as acquire. The lock token is correctly passed for authorization, and TTL=0 signals lock release to the storage API.
test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cs (11)
79-84: LGTM!Dispose correctly stops and disposes the WireMock server, then disposes the service provider. Order is appropriate.
105-157: LGTM!Well-structured happy path test that verifies the complete lock lifecycle: acquire → use → release. The request matching verification ensures correct HTTP interactions.
159-213: LGTM!Good test verifying that multiple
LockAsync()calls are idempotent—only one acquire request is made regardless of how many timesLockAsync()is invoked.
215-246: LGTM!Critical test ensuring locks are properly released even when exceptions occur. The
await usingstatement correctly triggersDisposeAsyncon the scope, which in turn disposes theInstanceLockerand releases the lock.
248-281: LGTM!Good test verifying that custom TTL values are correctly propagated to the storage API request body. The snapshot verification ensures the request body structure is as expected.
283-312: LGTM!Important test verifying that lock release failures don't propagate exceptions. This aligns with
InstanceLocker.DisposeAsyncbehavior of catching and logging release errors to prevent cascading failures.
314-338: LGTM!Good parameterized test covering multiple HTTP error statuses. The
UseParametersensures distinct snapshots per status code, and ignoring headers keeps snapshots stable across environments.
340-365: LGTM!Good edge case test verifying that a JSON
nullresponse body is handled correctly by throwingPlatformHttpResponseSnapshotExceptionrather than silently failing.
367-392: LGTM!Good edge case test verifying that an empty JSON object
{}(missingLockToken) is handled correctly by throwingPlatformHttpResponseSnapshotException.
394-415: LGTM!The test correctly overrides the
IHttpContextAccessorwith invalid route values and verifies thatInvalidOperationExceptionis thrown before any HTTP calls are made. The previous issue identified in code review has been addressed.
417-420: LGTM!Simple utility for generating predictable lock tokens in tests.
vxkc
added
feature
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs (1)
76-76: Static analysis: Generic catch clause is acceptable here.The static analysis tool flagged this as a "Generic catch clause." While technically catching
Exception, this uses awhenfilter to restrict handling to onlyJsonExceptionorInvalidOperationException, which is an idiomatic C# pattern for catching multiple specific exception types in a single block.This is acceptable and preferable to duplicate catch blocks. No action required.
🧹 Nitpick comments (1)
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs (1)
68-87: Consider preserving the original exception for better diagnostics.When the JSON parsing fails (lines 69-79), the original exception is logged but not included when throwing the
PlatformHttpResponseSnapshotExceptionon line 83. This loses the stack trace and detailed error information that could be valuable for debugging.Consider including the original exception as an inner exception:
🔎 Suggested improvement
- string? lockToken = null; + string? lockToken = null; + Exception? parseException = null; try { var lockResponse = await response.Content.ReadFromJsonAsync<InstanceLockResponse>( cancellationToken: cancellationToken ); lockToken = lockResponse?.LockToken; } catch (Exception e) when (e is JsonException || e is InvalidOperationException) { _logger.LogError(e, "Error reading response from the lock acquisition endpoint."); + parseException = e; } if (string.IsNullOrEmpty(lockToken)) { throw PlatformHttpResponseSnapshotException.Create( "The response from the lock acquisition endpoint was not expected.", - response + response, + parseException ); }Note: This assumes
PlatformHttpResponseSnapshotException.Createsupports an inner exception parameter. If not, this suggestion can be adapted to use constructor overload or alternative approach.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cstest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cstest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cs
**/test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/test/**/*.cs: Prefer xUnit asserts over FluentAssertions in tests
Mock external dependencies with Moq in tests
Files:
test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cs
🧠 Learnings (7)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs
📚 Learning: 2025-09-18T10:12:21.841Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1357
File: src/Altinn.App.Api/Infrastructure/Middleware/ScopeAuthorizationMiddleware.cs:289-293
Timestamp: 2025-09-18T10:12:21.841Z
Learning: In Altinn App security middleware like ScopeAuthorizationMiddleware, prefer explicit failures (throwing exceptions) over silently skipping unexpected endpoint types during discovery to prevent potential authorization gaps and ensure all endpoints are properly handled according to security configuration.
Applied to files:
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cs
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cs
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cs
🧬 Code graph analysis (2)
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs (4)
src/Altinn.App.Core/Features/Telemetry/Telemetry.InstanceLockClient.cs (1)
Telemetry(5-24)src/Altinn.App.Core/Configuration/PlatformSettings.cs (1)
PlatformSettings(7-74)src/Altinn.App.Core/Constants/General.cs (1)
General(6-42)src/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs (1)
instanceOwnerPartyId(40-55)
test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cs (4)
test/Altinn.App.Tests.Common/Auth/TestAuthentication.cs (1)
TestAuthentication(103-665)src/Altinn.App.Core/Configuration/PlatformSettings.cs (1)
PlatformSettings(7-74)src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs (4)
InstanceLockClient(16-111)InstanceLockClient(25-40)Task(42-90)Task(92-110)src/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs (1)
InstanceLocker(7-89)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Analyze (csharp)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Static code analysis
🔇 Additional comments (6)
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs (2)
25-40: LGTM: Constructor follows typed HttpClient pattern correctly.The constructor properly configures the injected HttpClient with platform settings and headers. This is the correct pattern for typed HttpClient usage with IHttpClientFactory, where each instance receives its own HttpClient to configure.
92-110: LGTM: Release method is well-structured.The lock release implementation correctly uses PATCH with TTL=0, properly handles errors by throwing
PlatformHttpResponseSnapshotException, and integrates with telemetry. The exception propagation is appropriate sinceInstanceLocker.DisposeAsynccatches and logs failures during release.test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cs (4)
24-103: LGTM: Well-structured test fixture with proper resource management.The
Fixtureclass provides a clean, reusable setup for integration tests with WireMock, proper DI configuration, and correct disposal of resources. The helper methods for building request matchers reduce duplication across tests.
105-213: LGTM: Comprehensive happy path coverage with proper async disposal.Both tests correctly exercise the lock lifecycle using
await usingfor proper disposal, which triggers lock release. TheHappyPath_MultipleLockCallstest valuably validates idempotency—ensuring multipleLockAsynccalls only acquire the lock once, preventing redundant lock operations.
215-392: LGTM: Thorough error handling and edge case coverage.The test suite comprehensively validates error scenarios:
- Exception-triggered lock release via
IAsyncDisposablepattern- TTL customization
- Silent failure on release errors (aligning with
InstanceLocker.DisposeAsyncbehavior)- Various HTTP error responses with parameterized testing
- Invalid response body handling
The use of snapshot testing with
Verifyfor exceptions provides good regression protection.
394-415: LGTM: Invalid instance identifier handling correctly validated.This test properly validates the error behavior when route values contain unparseable instance identifiers. The test correctly uses the configured
httpContextwith invalid route values (previously fixed per past review) and the test name matches the asserted exception type.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs (1)
258-259: Lock acquisition lacks cancellation support.The
CancellationToken ctparameter is available but not passed toLockAsync(), preventing graceful cancellation if the request is cancelled. This is the same issue flagged in a previous review comment.test/Altinn.App.Integration.Tests/InstanceLocking/InstanceLockTests.cs (1)
38-48: Race condition in test assertion - already flagged.The test assumes
Task.WhenAnyreturns the task with the conflict response, but either concurrent request could complete first. The first-completed task isn't guaranteed to be the one that got the lock conflict.src/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs (1)
57-77: Disposal with intentional exception swallowing.The generic catch clause is appropriate here - lock release failures during disposal should be logged but not propagated, as this could mask the original exception that triggered disposal. The CodeQL notice can be safely ignored.
Note: The
_lock = nullassignment afterLogLockReleasedis unreachable if release fails (due toreturnon line 71). Consider moving it into afinallyblock if clearing state on failure is desired.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
Directory.Packages.propssrc/Altinn.App.Core/Extensions/HttpClientExtension.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Features/Telemetry/Telemetry.InstanceLockClient.cssrc/Altinn.App.Core/Helpers/PlatformHttpResponseSnapshotException.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cssrc/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cssrc/Altinn.App.Core/Internal/Process/ProcessEngine.cstest/Altinn.App.Api.Tests/Controllers/Conventions/EnumSerializationTests.cstest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.CustomTtl_UsedInStorageApiCall.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.EmptyJsonResponseBody_ThrowsPlatformHttpException.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.InvalidInstanceId_ThrowsInvalidOperationException.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.NullResponseBody_ThrowsPlatformHttpException.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=Conflict.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=InternalServerError.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=NotFound.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Integration.Tests/InstanceLocking/InstanceLockTests.cstest/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/instance-lock/services/WaitForReleaseProcessTaskEnd.cs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.cs: Use CSharpier for code formatting (required before commits). Formatting happens automatically when building due to CSharpier.MSBuild
Use internal accessibility on types by default
Use sealed for classes unless inheritance is considered a valid use-case
Use Nullable Reference Types in C#
Remember to dispose IDisposable/IAsyncDisposable instances
For HTTP APIs, define ...Request and ...Response DTOs (e.g., LookupPersonRequest.cs and corresponding response)
Types meant to be implemented by apps should be marked with the ImplementableByApps attribute
Don't use .GetAwaiter().GetResult(), .Result(), .Wait() or other blocking APIs on Task
Don't use Async suffix for async methods
Write efficient code: Don't allocate unnecessarily (e.g., avoid calling ToString twice in a row, store in a variable; sometimes a for loop is better than LINQ)
Don't invoke the same async operation multiple times in the same codepath unless necessary
Don't await async operations in a loop (prefer batching, but maintain an upper bound on parallelism that makes sense)
Use strongly-typed configuration classes instead of untyped configuration
Register services in DI container properly following existing patterns
Files:
test/Altinn.App.Api.Tests/Controllers/Conventions/EnumSerializationTests.cssrc/Altinn.App.Core/Features/Telemetry/Telemetry.InstanceLockClient.cstest/Altinn.App.Integration.Tests/InstanceLocking/InstanceLockTests.cssrc/Altinn.App.Core/Helpers/PlatformHttpResponseSnapshotException.cssrc/Altinn.App.Core/Internal/Process/ProcessEngine.cssrc/Altinn.App.Core/Extensions/HttpClientExtension.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cstest/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/instance-lock/services/WaitForReleaseProcessTaskEnd.cstest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs
**/test/**/*.cs
📄 CodeRabbit inference engine (CLAUDE.md)
**/test/**/*.cs: Prefer xUnit asserts over FluentAssertions in tests
Mock external dependencies with Moq in tests
Files:
test/Altinn.App.Api.Tests/Controllers/Conventions/EnumSerializationTests.cstest/Altinn.App.Integration.Tests/InstanceLocking/InstanceLockTests.cstest/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/instance-lock/services/WaitForReleaseProcessTaskEnd.cstest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cs
🧠 Learnings (18)
📓 Common learnings
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
📚 Learning: 2025-09-21T08:19:43.290Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:1414-1414
Timestamp: 2025-09-21T08:19:43.290Z
Learning: In Altinn/app-lib-dotnet, *.verified.txt files (e.g., test/**/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt) are verification snapshots for guarding public API. Do not generate review suggestions based on these files; ignore them when assessing code changes.
Applied to files:
test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.InvalidInstanceId_ThrowsInvalidOperationException.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=NotFound.verified.txttest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=InternalServerError.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.NullResponseBody_ThrowsPlatformHttpException.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.EmptyJsonResponseBody_ThrowsPlatformHttpException.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cstest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.CustomTtl_UsedInStorageApiCall.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=Conflict.verified.txt
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn.App.Integration.Tests project, xUnit attributes like [Fact] are available without explicit "using Xunit;" directives, likely through global usings or implicit usings configuration. The code compiles and works as-is.
Applied to files:
test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.InvalidInstanceId_ThrowsInvalidOperationException.verified.txttest/Altinn.App.Integration.Tests/InstanceLocking/InstanceLockTests.cssrc/Altinn.App.Core/Internal/Process/ProcessEngine.cstest/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/instance-lock/services/WaitForReleaseProcessTaskEnd.cstest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-08-29T05:32:47.222Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/AppFixture.cs:958-958
Timestamp: 2025-08-29T05:32:47.222Z
Learning: In Altinn.App.Integration.Tests, the maintainer prefers fail-fast behavior (Environment.FailFast) when critical test infrastructure components like log reading fail, rather than graceful error handling, because diagnostics are essential for debugging test failures.
Applied to files:
test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.InvalidInstanceId_ThrowsInvalidOperationException.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=NotFound.verified.txttest/Altinn.App.Integration.Tests/InstanceLocking/InstanceLockTests.cstest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=InternalServerError.verified.txt
📚 Learning: 2025-10-17T07:45:15.474Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 1474
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:233-237
Timestamp: 2025-10-17T07:45:15.474Z
Learning: In Altinn/app-lib-dotnet, maintainers accept source compatibility without guaranteeing binary compatibility across versions because consumers upgrade via NuGet and rebuild. Adding optional parameters to public extension methods (e.g., CancellationToken) is acceptable provided release notes document the change.
Applied to files:
test/Altinn.App.Api.Tests/Controllers/Conventions/EnumSerializationTests.cstest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txtsrc/Altinn.App.Core/Helpers/PlatformHttpResponseSnapshotException.cssrc/Altinn.App.Core/Internal/Process/ProcessEngine.cssrc/Altinn.App.Core/Extensions/HttpClientExtension.cssrc/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.csDirectory.Packages.props
📚 Learning: 2025-08-22T13:46:43.017Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1446
File: test/Altinn.App.Integration.Tests/Basic/_snapshots/BasicAppTests.Full_auth=ServiceOwner_testCase=MultipartXmlPrefill_7_Logs.verified.txt:41-42
Timestamp: 2025-08-22T13:46:43.017Z
Learning: In Altinn App Integration Tests, OldUser and OldServiceOwner test snapshots intentionally preserve the legacy "AuthMethod: localtest" authentication method as part of a migration strategy, while new User and ServiceOwner tests use updated authentication methods like BankID and maskinporten. The "Old*" variants should not be updated to remove localtest references.
Applied to files:
test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=NotFound.verified.txttest/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txttest/Altinn.App.Integration.Tests/InstanceLocking/InstanceLockTests.cstest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.NullResponseBody_ThrowsPlatformHttpException.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.EmptyJsonResponseBody_ThrowsPlatformHttpException.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cstest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.CustomTtl_UsedInStorageApiCall.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=Conflict.verified.txt
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: New features should follow the established pattern in /src/Altinn.App.Core/Features/ with feature-specific folders, DI registration, telemetry, and test coverage
Applied to files:
test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=NotFound.verified.txtsrc/Altinn.App.Core/Internal/Process/ProcessEngine.cstest/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/instance-lock/services/WaitForReleaseProcessTaskEnd.cstest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-09-07T20:03:48.030Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1463
File: test/Altinn.App.SourceGenerator.Tests/DiagnosticTests.RunJsonError.verified.txt:1-21
Timestamp: 2025-09-07T20:03:48.030Z
Learning: In Altinn.App.SourceGenerator.Tests, the path C:\temp\config\applicationmetadata.json is a hardcoded test fixture path required by Roslyn and is used consistently across all operating systems, not an actual filesystem path that varies by OS.
Applied to files:
test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=NotFound.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.StorageApiError_ThrowsCorrectPlatformHttpException_storageStatusCode=InternalServerError.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.EmptyJsonResponseBody_ThrowsPlatformHttpException.verified.txttest/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.CustomTtl_UsedInStorageApiCall.verified.txt
📚 Learning: 2025-09-25T08:15:25.624Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt:3656-3671
Timestamp: 2025-09-25T08:15:25.624Z
Learning: In PR Altinn/app-lib-dotnet#745, the team decided to defer adding idempotency documentation, enriched ServiceTaskFailedResult (code/title/message), and additional OTel spans for service tasks. Do not re-suggest these changes within this PR; propose a follow-up issue instead if needed.
Applied to files:
test/Altinn.App.Integration.Tests/InstanceLocking/InstanceLockTests.cstest/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/instance-lock/services/WaitForReleaseProcessTaskEnd.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Follow the AppFixture pattern for integration tests: Central orchestrator managing test lifecycle with feature-specific operations
Applied to files:
test/Altinn.App.Integration.Tests/InstanceLocking/InstanceLockTests.cs
📚 Learning: 2025-09-21T09:04:43.977Z
Learnt from: ivarne
Repo: Altinn/app-lib-dotnet PR: 1473
File: src/Altinn.App.Core/Features/IInstanceDataAccessor.cs:36-41
Timestamp: 2025-09-21T09:04:43.977Z
Learning: The IInstanceDataAccessor interface in Altinn.App.Core is designed for consumption only - users are not expected to implement this interface themselves, only use framework-provided implementations.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cssrc/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs
📚 Learning: 2025-08-29T10:45:57.158Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1456
File: test/Altinn.App.Integration.Tests/_fixture/Tests.cs:3-4
Timestamp: 2025-08-29T10:45:57.158Z
Learning: In Altinn app-lib-dotnet projects, ImplicitUsings is enabled in Directory.Build.props, which automatically includes common using statements including Xunit namespace for test projects. This eliminates the need for explicit "using Xunit;" statements in test files.
Applied to files:
src/Altinn.App.Core/Internal/Process/ProcessEngine.cssrc/Altinn.App.Core/Extensions/ServiceCollectionExtensions.csDirectory.Packages.props
📚 Learning: 2025-09-18T10:12:21.841Z
Learnt from: martinothamar
Repo: Altinn/app-lib-dotnet PR: 1357
File: src/Altinn.App.Api/Infrastructure/Middleware/ScopeAuthorizationMiddleware.cs:289-293
Timestamp: 2025-09-18T10:12:21.841Z
Learning: In Altinn App security middleware like ScopeAuthorizationMiddleware, prefer explicit failures (throwing exceptions) over silently skipping unexpected endpoint types during discovery to prevent potential authorization gaps and ensure all endpoints are properly handled according to security configuration.
Applied to files:
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cssrc/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Include integration tests for platform service interactions
Applied to files:
test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/instance-lock/services/WaitForReleaseProcessTaskEnd.cs
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Register services in DI container properly following existing patterns
Applied to files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which supports default interface methods.
Applied to files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.csDirectory.Packages.props
📚 Learning: 2025-09-29T08:34:53.864Z
Learnt from: bjorntore
Repo: Altinn/app-lib-dotnet PR: 745
File: src/Altinn.App.Core/EFormidling/Interface/IEFormidlingService.cs:18-30
Timestamp: 2025-09-29T08:34:53.864Z
Learning: Altinn.App.Core project targets net8.0, not netstandard2.0, which fully supports default interface methods.
Applied to files:
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.csDirectory.Packages.props
📚 Learning: 2025-11-28T08:07:52.106Z
Learnt from: CR
Repo: Altinn/app-lib-dotnet PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-28T08:07:52.106Z
Learning: Applies to **/*.cs : Remember to dispose IDisposable/IAsyncDisposable instances
Applied to files:
src/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs
🧬 Code graph analysis (6)
src/Altinn.App.Core/Features/Telemetry/Telemetry.InstanceLockClient.cs (1)
src/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs (1)
instanceOwnerPartyId(40-55)
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs (1)
src/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs (1)
InstanceLocker(7-89)
src/Altinn.App.Core/Extensions/HttpClientExtension.cs (3)
src/Altinn.App.Core/Infrastructure/Clients/AccessManagement/AccessManagementClient.cs (1)
HttpRequestMessage(132-142)src/Altinn.App.Core/Constants/AuthorizationSchemes.cs (1)
AuthorizationSchemes(6-12)src/Altinn.App.Core/Constants/General.cs (1)
General(6-42)
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs (2)
src/Altinn.App.Core/Features/Telemetry/Telemetry.InstanceLockClient.cs (1)
Telemetry(5-24)src/Altinn.App.Core/Constants/General.cs (1)
General(6-42)
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs (2)
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs (2)
InstanceLockClient(16-111)InstanceLockClient(25-40)src/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs (1)
InstanceLocker(7-89)
src/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs (1)
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs (2)
InstanceLockClient(16-111)InstanceLockClient(25-40)
🪛 GitHub Actions: Build and Test on windows, macos and ubuntu
src/Altinn.App.Core/Internal/Process/ProcessEngine.cs
[error] 79-79: No service for type 'Altinn.App.Core.Internal.InstanceLocking.InstanceLocker' has been registered. Dependency injection cannot resolve InstanceLocker when constructing ProcessEngine.
src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs
[error] 61-65: AcquireInstanceLockHTTP call failed with 503 Service Unavailable in test environment. Not mocked HTTP request for storage/lock endpoint. Not mocking HttpClient in test setup.
🪛 GitHub Check: CodeQL
src/Altinn.App.Core/Helpers/PlatformHttpResponseSnapshotException.cs
[notice] 91-94: Generic catch clause
Generic catch clause.
src/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs
[notice] 68-72: Generic catch clause
Generic catch clause.
🔇 Additional comments (23)
test/Altinn.App.Api.Tests/Controllers/Conventions/EnumSerializationTests.cs (1)
89-96: LGTM - Formatting and type qualification improvement.The fully qualified return type and cast improve code clarity without changing behavior. The multi-line parameter formatting also enhances readability.
src/Altinn.App.Core/Extensions/ServiceCollectionExtensions.cs (2)
118-118: LGTM - HttpClient registration follows established pattern.The
AddHttpClient<InstanceLockClient>()registration is consistent with other storage clients in this file and properly placed inAddPlatformServices.
369-369: LGTM - Scoped lifetime is appropriate for InstanceLocker.Registering
InstanceLockeras scoped ensures:
- One instance per HTTP request
- Proper disposal of the
IAsyncDisposableat request end (releasing the lock)- Lock reuse within the same request if
LockAsyncis called multiple times- Access to
HttpContextthrough the scopedIHttpContextAccessorThe placement in
AddProcessServicesaligns with its usage inProcessEngine.src/Altinn.App.Core/Internal/Process/ProcessEngine.cs (3)
9-9: LGTM!The using directive correctly imports the namespace for InstanceLocker.
46-46: LGTM!The field declaration follows proper conventions and is marked readonly.
79-79: > Likely an incorrect or invalid review comment.src/Altinn.App.Core/Features/Telemetry/Telemetry.InstanceLockClient.cs (1)
1-24: LGTM!The telemetry helper methods follow established patterns for activity instrumentation. Both methods correctly:
- Use internal accessibility
- Create activities with descriptive names
- Set appropriate tags for observability
- Return nullable Activity to handle cases where activity creation fails
The implementation is clean and focused on its single responsibility.
test/Altinn.App.Core.Tests/PublicApiTests.PublicApi_ShouldNotChange_Unintentionally.verified.txt (1)
245-247: No review needed for snapshot contentThis file is a generated public-API snapshot; the added
HttpClientExtension.PatchAsyncentry simply records the updated surface. No semantic review or suggestions here.
Based on learnings, verified snapshot files are not a basis for review feedback.src/Altinn.App.Core/Extensions/HttpClientExtension.cs (1)
147-180: LGTM! Consistent with existing extension methods.The new
PatchAsyncextension follows the same pattern asPostAsyncandPutAsync, properly handling authorization headers and optional platform access tokens.test/Altinn.App.Integration.Tests/_testapps/basic/_scenarios/instance-lock/services/WaitForReleaseProcessTaskEnd.cs (2)
13-32: LGTM! Well-designed test coordination mechanism.The static
TaskCompletionSourcepattern is appropriate for coordinating async operations across HTTP requests in integration tests. TheReset()method correctly signals existing waiters before creating a new TCS.
34-65: Test endpoints and DI registration look good.The test endpoints provide clean control over the wait/release lifecycle, and the DI registration follows the established pattern for test scenarios.
src/Altinn.App.Core/Helpers/PlatformHttpResponseSnapshotException.cs (2)
83-95: LGTM! Intentional swallowing of disposal exceptions.The generic catch clause at lines 91-94 is appropriate here. Disposal failures during exception handling should not mask the original exception. The CodeQL notice can be safely ignored as this is a deliberate pattern for cleanup safety.
108-166: Clean factory method with proper header sanitization.The new
Createmethod properly:
- Builds a sanitized, non-streaming response copy
- Redacts sensitive headers (Authorization, Cookie, etc.)
- Handles trailing headers for HTTP/2+
- Preserves content metadata
src/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs (2)
7-21: LGTM! Clean primary constructor and lock interface.The constructor properly validates
HttpContextavailability, and the parameterlessLockAsync()provides a sensible 5-minute default TTL.
23-38: Idempotent lock acquisition is correct.The early return when
_lock is not nullprevents duplicate lock acquisition attempts, which is the expected behavior for a scoped locker.src/Altinn.App.Core/Infrastructure/Clients/Storage/InstanceLockClient.cs (3)
25-40: LGTM! Standard HttpClient configuration.The constructor properly configures the HttpClient with base URL, subscription key header, and JSON accept header following the established pattern in other storage clients.
42-90: Lock acquisition with appropriate error handling.The method:
- Uses telemetry for observability
- Properly handles non-success responses with
PlatformHttpResponseSnapshotException- Catches specific JSON parsing exceptions rather than generic
Exception- Validates the lock token before returning
92-110: Lock token is correctly used as authorization for lock release.The implementation matches the intended API contract. The
lockTokenis passed as theauthorizationTokenparameter toPatchAsyncon line 104, placing it in the Authorization header with Bearer scheme. Tests confirm this is the expected behavior: only the holder of the lock token can release a lock, which is the correct security model.test/Altinn.App.Core.Tests/Internal/InstanceLocking/InstanceLockTests.cs (5)
24-103: Well-structured test fixture with WireMock.The
Fixtureclass properly:
- Manages WireMock server lifecycle
- Configures DI with appropriate mocks
- Provides request builders that match the client implementation
- Implements
IDisposablefor cleanup
105-157: Comprehensive happy path test.The test validates the complete lock lifecycle: acquire → perform operation → release. Request matching ensures the correct endpoints are called in the expected order.
159-246: Good coverage of edge cases.
HappyPath_MultipleLockCalls: Validates idempotent lock behaviorLockReleasedOnException: Ensures cleanup on failureBoth are critical scenarios for a locking mechanism.
314-392: Thorough error handling coverage.The parameterized tests cover various HTTP error codes and malformed responses, ensuring
PlatformHttpResponseSnapshotExceptionis thrown appropriately.
394-415: InvalidInstanceId test correctly configured.The test now properly uses the configured
httpContextwith invalid route values (per past review feedback). The test verifies thatInvalidOperationExceptionis thrown when instance identifiers cannot be extracted.
| await _instanceLocker.LockAsync(); | ||
|
|
There was a problem hiding this comment.
Critical: Lock is never released, causing resource leaks.
The lock is acquired but InstanceLocker (which implements IAsyncDisposable) is never disposed. This violates the coding guideline to "Remember to dispose IDisposable/IAsyncDisposable instances" and will cause locks to remain held until the service instance is disposed.
As noted in the PR comments by ivarne, the lock should be managed using either:
- A
usingstatement with anIAsyncDisposablelock object, or - An explicit
try { } finally { }block to ensure release
Consider restructuring so that InstanceLocker is resolved per-request and used with await using rather than as a field:
await using var locker = serviceProvider.GetRequiredService<InstanceLocker>();
await locker.LockAsync();
// ... rest of processing
// Automatic disposal releases lock3 participants
Description
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Tests
Dependencies
✏️ Tip: You can customize this high-level summary in your review settings.