feat: include lock token header in instance data mutations#1699
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| catch (Exception e) | ||
| { | ||
| activity?.Errored(e); | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 months ago
In general, the fix is to avoid catching Exception and instead catch only those specific exception types that are expected when contacting the storage service to release the lock. Unexpected or critical exceptions should not be silently swallowed; they should bubble up so they can be observed and handled at a higher level.
For this code, the safest, functionality-preserving change is to replace catch (Exception e) with one or more specific catch clauses for expected operational failures of client.UpdateInstanceLock. Because this is I/O to a storage backend (likely over HTTP), expected failures are typically network and timeout issues (e.g., HttpRequestException, TaskCanceledException, TimeoutException). These are the conditions where we really want “best-effort” behavior: if we fail to release the lock because of transient connectivity issues, we log the failure in telemetry and allow the lock to expire naturally.
Concretely:
- In
src/Altinn.App.Core/Internal/InstanceLocking/InstanceLocker.cs, withinInstanceLockHandle.DisposeAsync, replace the genericcatch (Exception e)with multiple, more specific catch clauses:catch (HttpRequestException e)catch (TaskCanceledException e)(coversOperationCanceledException-like cancellation of the HTTP call)catch (TimeoutException e)
- Each of these catch blocks should retain the existing
activity?.Errored(e);call, preserving current telemetry behavior. - Do not change other logic (e.g., we still do not rethrow; we still only clear
holder.LockTokenon success). - Add
using System.Net.Http;at the top of the file to referenceHttpRequestException.TaskCanceledExceptionandTimeoutExceptionare inSystem, already implicitly available.
This keeps behavior the same for expected communication failures while allowing other, unexpected exceptions to be visible instead of being silently eaten.
| @@ -1,6 +1,7 @@ | ||
| using Altinn.App.Core.Features; | ||
| using Altinn.App.Core.Infrastructure.Clients.Storage; | ||
| using Microsoft.AspNetCore.Http; | ||
| using System.Net.Http; | ||
|
|
||
| namespace Altinn.App.Core.Internal.InstanceLocking; | ||
|
|
||
| @@ -123,10 +124,18 @@ | ||
|
|
||
| holder.LockToken = null; | ||
| } | ||
| catch (Exception e) | ||
| catch (HttpRequestException e) | ||
| { | ||
| activity?.Errored(e); | ||
| } | ||
| catch (TaskCanceledException e) | ||
| { | ||
| activity?.Errored(e); | ||
| } | ||
| catch (TimeoutException e) | ||
| { | ||
| activity?.Errored(e); | ||
| } | ||
| } | ||
| } | ||
| } |
|
2 participants
Description
Related Issue(s)
Verification
Documentation