Conversation
📝 WalkthroughWalkthroughException handling in InstancesController was updated to perform best-effort hard deletion of partially created instances on various failure paths (Post, PostSimplified, CopyInstance). Deletion attempts are guarded, failures are logged, and original errors are still returned. A new private helper TryDeleteInstance(...) centralizes deletion logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🧹 Nitpick comments (1)
src/Altinn.App.Api/Controllers/InstancesController.cs (1)
880-896: Consider adding similar cleanup logic toCopyInstancefor consistency.The
CopyInstancemethod creates an instance (line 880) and then performs additional operations that could fail (lines 888-896). If these fail, the instance will be left in an orphaned state, similar to the issue being fixed inPostandPostSimplified.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Api/Controllers/InstancesController.cs` around lines 880 - 896, After creating the target instance in CopyInstance (the call to _instanceClient.CreateInstance), wrap the subsequent operations (CopyDataFromSourceInstance, _instanceClient.GetInstance, and _processEngine.HandleEventsAndUpdateStorage) in a try/catch and on any exception delete the newly created instance to avoid orphaned instances; call the instance-delete API (e.g., _instanceClient.DeleteInstance or the controller's existing instance removal helper) with the created targetInstance and appropriate authentication/CancellationToken before rethrowing the exception so cleanup mirrors the logic used in Post/PostSimplified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Altinn.App.Api/Controllers/InstancesController.cs`:
- Around line 427-433: When attempting cleanup in InstancesController, wrap the
call to _instanceClient.DeleteInstance(...) (using
instance.InstanceOwner.PartyId and instance.Id) in its own try/catch so any
exception from DeleteInstance is logged but does not replace the original
instantiation exception; i.e., inside the catch that handles the failed
instantiation, call DeleteInstance inside try { await
_instanceClient.DeleteInstance(...); } catch (Exception delEx) {
_logger.LogWarning(delEx, "Cleanup DeleteInstance failed for instance
{InstanceId}"); } and then rethrow or propagate the original instantiation
exception (or use ExceptionDispatchInfo.Capture(originalEx).Throw() if
rethrowing) so the original error is preserved.
- Around line 733-744: The deletion call to _instanceClient.DeleteInstance
inside InstancesController should be wrapped in a try-catch so deletion failures
are logged and do not mask the original exception; update the code around the
existing if (instance?.Id is not null) block to catch Exception (e.g., catch
(Exception ex)) when calling _instanceClient.DeleteInstance, log the error with
contextual info (include instance.InstanceOwner.PartyId and instance.Id or the
parsed GUID) via the controller's logger (e.g., _logger.LogError) and do not
rethrow so the original exception remains the primary failure.
---
Nitpick comments:
In `@src/Altinn.App.Api/Controllers/InstancesController.cs`:
- Around line 880-896: After creating the target instance in CopyInstance (the
call to _instanceClient.CreateInstance), wrap the subsequent operations
(CopyDataFromSourceInstance, _instanceClient.GetInstance, and
_processEngine.HandleEventsAndUpdateStorage) in a try/catch and on any exception
delete the newly created instance to avoid orphaned instances; call the
instance-delete API (e.g., _instanceClient.DeleteInstance or the controller's
existing instance removal helper) with the created targetInstance and
appropriate authentication/CancellationToken before rethrowing the exception so
cleanup mirrors the logic used in Post/PostSimplified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3452a484-ae92-4420-a100-71179f1025aa
📒 Files selected for processing (1)
src/Altinn.App.Api/Controllers/InstancesController.cs
ivarne
added
backport
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Altinn.App.Api/Controllers/InstancesController.cs (1)
744-770:⚠️ Potential issue | 🔴 CriticalMissing closing brace for the
ifblock causes compilation failure.The
if (instance?.Id is not null)block opened on line 744 is never closed. Line 765 closes the innercatchblock, but the outerifblock remains unclosed before thereturnstatement. This is the cause of the pipeline failure.🐛 Proposed fix to add missing closing brace
catch (Exception deleteException) { _logger.LogError( deleteException, "Failed to delete instance {InstanceId} after unsuccessful instantiation. Manual cleanup might be required.", instance.Id ); + } } return ExceptionResponse( exception, $"Instantiation of appId {org}/{app} failed for party {instanceTemplate.InstanceOwner?.PartyId}" ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Altinn.App.Api/Controllers/InstancesController.cs` around lines 744 - 770, Add the missing closing brace to properly close the if (instance?.Id is not null) block that wraps the try/catch which calls _instanceClient.DeleteInstance(...); specifically insert a '}' after the catch block (just before the existing return ExceptionResponse(...) line) so the try/catch is enclosed by the if and compilation succeeds in the InstancesController class.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Altinn.App.Api/Controllers/InstancesController.cs`:
- Around line 744-770: Add the missing closing brace to properly close the if
(instance?.Id is not null) block that wraps the try/catch which calls
_instanceClient.DeleteInstance(...); specifically insert a '}' after the catch
block (just before the existing return ExceptionResponse(...) line) so the
try/catch is enclosed by the if and compilation succeeds in the
InstancesController class.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ed446319-b570-4da7-aea0-757b11a18a74
📒 Files selected for processing (1)
src/Altinn.App.Api/Controllers/InstancesController.cs
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Altinn.App.Api/Controllers/InstancesController.cs`:
- Around line 427-444: The catch path currently calls
_instanceClient.DeleteInstance directly (and may attempt deletion twice) and
returns ExceptionResponse instead of the original prefillProblem; refactor so
both the normal path and the catch path call a single best-effort cleanup helper
(e.g., TryCleanupInstanceAsync(instance) that wraps
_instanceClient.DeleteInstance and logs errors) and ensure the handler returns
the original prefillProblem (not ExceptionResponse) after invoking the helper;
update references to _instanceClient.DeleteInstance, deleteException, and
prefillProblem accordingly so deletion is centralized and not retried twice.
- Around line 744-766: The CopyInstance flow still lacks the best-effort orphan
cleanup: wrap the create/copy/process sequence that calls
CopyDataFromSourceInstance(...) and HandleEventsAndUpdateStorage(...) in a
try/catch and on any exception attempt the same delete logic used elsewhere
(parse party id from instance.InstanceOwner.PartyId and instance id GUID from
instance.Id.Split("/")[1], call _instanceClient.DeleteInstance(..., hard: true,
authenticationMethod: null, CancellationToken.None)), checking instance?.Id is
not null before deleting; if the delete throws, log it with _logger.LogError
including the delete exception and the instance.Id for diagnostics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0690ff1a-fcf5-41d5-b9de-61e3010b1245
📒 Files selected for processing (1)
src/Altinn.App.Api/Controllers/InstancesController.cs
| catch (Exception deleteException) | ||
| { | ||
| _logger.LogError( | ||
| deleteException, | ||
| "Failed to delete instance {InstanceId} after unsuccessful instantiation. Manual cleanup might be required.", | ||
| instance.Id | ||
| ); | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 22 days ago
In general, the fix is to replace the generic catch (Exception) used for the delete‑cleanup with one or more specific exception types that represent expected, non‑fatal failures from _instanceClient.DeleteInstance, and optionally add a final catch that only rethrows or does minimal handling. Given the surrounding code already uses PlatformHttpException when talking to the instance client, the most appropriate targeted catch type here is PlatformHttpException. That exception indicates a failure in the underlying platform HTTP call (e.g., instance not found, forbidden, server error) and is precisely what we want to log and ignore in best‑effort cleanup.
Concretely, within src/Altinn.App.Api/Controllers/InstancesController.cs, in the inner try/catch responsible for deleting the instance after a failed instantiation, change catch (Exception deleteException) to catch (PlatformHttpException deleteException). This does not alter control flow: we still try to delete, and on failure we log the error and continue to return the original exception via ExceptionResponse. No new methods or imports are required because PlatformHttpException is already in use earlier in the same method and its namespace is already imported.
| @@ -732,7 +732,7 @@ | ||
| CancellationToken.None | ||
| ); | ||
| } | ||
| catch (Exception deleteException) | ||
| catch (PlatformHttpException deleteException) | ||
| { | ||
| _logger.LogError( | ||
| deleteException, |
| catch (Exception exception) | ||
| { | ||
| await TryDeleteInstance(targetInstance); | ||
|
|
||
| string url = SelfLinkHelper.BuildFrontendSelfLink(targetInstance, Request); | ||
| return ExceptionResponse( | ||
| exception, | ||
| $"Copying instance {instanceOwnerPartyId}/{instanceGuid} failed for party {targetInstance?.InstanceOwner?.PartyId}" | ||
| ); | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 22 days ago
General approach: narrow the generic catch so that clearly distinct categories of exceptions are either rethrown or handled separately, and only “expected” operational failures are handled by the current ExceptionResponse logic. For controller actions that support cancellation tokens, it is especially important not to swallow OperationCanceledException (and related exceptions), to avoid blocking cooperative cancellation and to let ASP.NET Core handle aborted requests appropriately.
Concrete best fix here without changing existing functionality for normal failures:
- Add a specific
catch (OperationCanceledException)block before the generic catch. In this block, rethrow usingthrow;so that framework-level cancellation handling remains intact. If you prefer explicit behavior you could translate it into a 499/Client Closed Request or similar, but that would change externally visible behavior; simply rethrow is safer. - Optionally consider other special cases (like
TaskCanceledExceptionif used directly), butOperationCanceledExceptionis usually sufficient because most task cancellation surfaces as this type. - Keep the existing
catch (Exception exception)block for all other exceptions, including the cleanup (TryDeleteInstance) and the call toExceptionResponse, to avoid changing the error contract.
All changes are within src/Altinn.App.Api/Controllers/InstancesController.cs in the shown method, around lines 879–917. No new imports are required because OperationCanceledException is in System, which is implicitly available.
| @@ -906,6 +906,11 @@ | ||
|
|
||
| return Redirect(url); | ||
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| // Preserve cancellation semantics by letting the exception propagate | ||
| throw; | ||
| } | ||
| catch (Exception exception) | ||
| { | ||
| await TryDeleteInstance(targetInstance); |
| catch (Exception deleteException) | ||
| { | ||
| _logger.LogError( | ||
| deleteException, | ||
| "Failed to delete instance {InstanceId} after unsuccessful copy. Manual cleanup might be required.", | ||
| targetInstance.Id | ||
| ); | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 22 days ago
In general, the issue is fixed by catching only the exception types you reasonably expect and can handle, and treating unexpected exceptions differently (e.g., letting them bubble up, or at least logging them as unexpected). Here, we likely need to keep the method from throwing (because it's a “try” cleanup method), but we can still narrow the main catch and then have a final catch (Exception) only as a last resort.
Best fix without changing existing functionality: introduce specific catches for common storage-related exceptions (such as FormatException if the IDs are malformed, ArgumentException for parameter issues, and potentially OperationCanceledException if cancellation could occur), log them as before, and then keep a final generic catch for anything else. To slightly reduce breadth while maintaining behavior, we can add a generic catch that logs with a more explicit “unexpected error” message but still swallows it. This ensures we haven't changed outward behavior (no new exceptions escape), but we make it clearer what we consider expected vs. unexpected issues and satisfy the intent of the CodeQL rule by avoiding a single broad catch.
Concretely, in src/Altinn.App.Api/Controllers/InstancesController.cs, around lines 920–941, we will:
- Add a more specific catch for
FormatException(in casePartyIdorIdparsing fails). - Add a more specific catch for
ArgumentException(defensive for bad arguments toDeleteInstance). - Optionally add a specific catch for
OperationCanceledExceptionif the client can throw it. - Keep a final
catch (Exception deleteException)as a safety net, but now it’s only for truly unexpected errors and can log accordingly.
No new using directives are required, since FormatException, ArgumentException, and OperationCanceledException are in System, which is already implicitly available.
| @@ -931,11 +931,35 @@ | ||
| CancellationToken.None | ||
| ); | ||
| } | ||
| catch (FormatException formatException) | ||
| { | ||
| _logger.LogError( | ||
| formatException, | ||
| "Failed to delete instance {InstanceId} after unsuccessful copy due to invalid identifier format. Manual cleanup might be required.", | ||
| targetInstance.Id | ||
| ); | ||
| } | ||
| catch (ArgumentException argumentException) | ||
| { | ||
| _logger.LogError( | ||
| argumentException, | ||
| "Failed to delete instance {InstanceId} after unsuccessful copy due to invalid argument. Manual cleanup might be required.", | ||
| targetInstance.Id | ||
| ); | ||
| } | ||
| catch (OperationCanceledException canceledException) | ||
| { | ||
| _logger.LogError( | ||
| canceledException, | ||
| "Deletion of instance {InstanceId} after unsuccessful copy was canceled. Manual cleanup might be required.", | ||
| targetInstance.Id | ||
| ); | ||
| } | ||
| catch (Exception deleteException) | ||
| { | ||
| _logger.LogError( | ||
| deleteException, | ||
| "Failed to delete instance {InstanceId} after unsuccessful copy. Manual cleanup might be required.", | ||
| "Unexpected error when deleting instance {InstanceId} after unsuccessful copy. Manual cleanup might be required.", | ||
| targetInstance.Id | ||
| ); | ||
| } |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Altinn.App.Api/Controllers/InstancesController.cs`:
- Around line 879-917: The try/catch currently surrounds post-create response
work so TryDeleteInstance(targetInstance) may hard-delete a copy after
successful HandleEventsAndUpdateStorage; restrict rollback to the creation/copy
initialization steps by enclosing only the sequence that can require rollback
(calls to _instanceClient.CreateInstance, CopyDataFromSourceInstance,
_instanceClient.GetInstance and _processEngine.HandleEventsAndUpdateStorage) in
the try, let RegisterEvent, SelfLinkHelper.BuildFrontendSelfLink and Redirect
happen after the try completes, and keep the catch calling
TryDeleteInstance(targetInstance) and returning ExceptionResponse(exception,
...) so that deletion only occurs for failures during initialization, not for
failures building/returning the response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: beb2b2db-5a67-4fb7-a600-fe0d3f8e63ce
📒 Files selected for processing (1)
src/Altinn.App.Api/Controllers/InstancesController.cs
| try | ||
| { | ||
| targetInstance = await _instanceClient.CreateInstance( | ||
| org, | ||
| app, | ||
| targetInstance, | ||
| authenticationMethod: null, | ||
| CancellationToken.None | ||
| ); | ||
|
|
||
| await CopyDataFromSourceInstance(application, targetInstance, sourceInstance); | ||
| await CopyDataFromSourceInstance(application, targetInstance, sourceInstance); | ||
|
|
||
| targetInstance = await _instanceClient.GetInstance( | ||
| targetInstance, | ||
| authenticationMethod: null, | ||
| CancellationToken.None | ||
| ); | ||
| targetInstance = await _instanceClient.GetInstance( | ||
| targetInstance, | ||
| authenticationMethod: null, | ||
| CancellationToken.None | ||
| ); | ||
|
|
||
| await _processEngine.HandleEventsAndUpdateStorage( | ||
| targetInstance, | ||
| null, | ||
| startResult.ProcessStateChange?.Events | ||
| ); | ||
|
|
||
| await RegisterEvent("app.instance.created", targetInstance); | ||
|
|
||
| await _processEngine.HandleEventsAndUpdateStorage(targetInstance, null, startResult.ProcessStateChange?.Events); | ||
| string url = SelfLinkHelper.BuildFrontendSelfLink(targetInstance, Request); | ||
|
|
||
| await RegisterEvent("app.instance.created", targetInstance); | ||
| return Redirect(url); | ||
| } | ||
| catch (Exception exception) | ||
| { | ||
| await TryDeleteInstance(targetInstance); | ||
|
|
||
| string url = SelfLinkHelper.BuildFrontendSelfLink(targetInstance, Request); | ||
| return ExceptionResponse( | ||
| exception, | ||
| $"Copying instance {instanceOwnerPartyId}/{instanceGuid} failed for party {targetInstance?.InstanceOwner?.PartyId}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Keep rollback scoped to copy initialization.
The catch also covers Lines 903-907, so any later exception in response construction will call TryDeleteInstance(targetInstance) and hard-delete a copy that already passed HandleEventsAndUpdateStorage(...). Move the post-create response work outside the rollback scope.
♻️ Proposed fix
try
{
targetInstance = await _instanceClient.CreateInstance(
org,
app,
@@
await _processEngine.HandleEventsAndUpdateStorage(
targetInstance,
null,
startResult.ProcessStateChange?.Events
);
-
- await RegisterEvent("app.instance.created", targetInstance);
-
- string url = SelfLinkHelper.BuildFrontendSelfLink(targetInstance, Request);
-
- return Redirect(url);
}
catch (Exception exception)
{
await TryDeleteInstance(targetInstance);
return ExceptionResponse(
exception,
$"Copying instance {instanceOwnerPartyId}/{instanceGuid} failed for party {targetInstance?.InstanceOwner?.PartyId}"
);
}
+
+ await RegisterEvent("app.instance.created", targetInstance);
+ string url = SelfLinkHelper.BuildFrontendSelfLink(targetInstance, Request);
+ return Redirect(url);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Altinn.App.Api/Controllers/InstancesController.cs` around lines 879 -
917, The try/catch currently surrounds post-create response work so
TryDeleteInstance(targetInstance) may hard-delete a copy after successful
HandleEventsAndUpdateStorage; restrict rollback to the creation/copy
initialization steps by enclosing only the sequence that can require rollback
(calls to _instanceClient.CreateInstance, CopyDataFromSourceInstance,
_instanceClient.GetInstance and _processEngine.HandleEventsAndUpdateStorage) in
the try, let RegisterEvent, SelfLinkHelper.BuildFrontendSelfLink and Redirect
happen after the try completes, and keep the catch calling
TryDeleteInstance(targetInstance) and returning ExceptionResponse(exception,
...) so that deletion only occurs for failures during initialization, not for
failures building/returning the response.
3 participants
When instantiation fails, we previously left a half initialised instance that likely misses the data element with appLogic and fails to open in our frontend.
This attempts to delete the instance if an exception happens in the init code, but we still have the issue of application crashes that might cause deletion to fail or the full app container to crash.
Summary by CodeRabbit