Garnet evaluation#2009
Conversation
📝 WalkthroughWalkthroughThe standalone Redis container is removed from ChangesEmbedded Garnet cache server
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ 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: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Altinn.Correspondence.API/Altinn.Correspondence.API.csproj`:
- Line 37: Update the PackageReference for Microsoft.Garnet to a version that
brings Snappier 1.3.1 or higher to address the high-severity Snappier
vulnerability (GHSA-pggp-6c3x-2xmx). Additionally, locate and update the
OpenTelemetry.Api PackageReference to version 1.15.3 or higher to resolve the
moderate-severity memory exhaustion vulnerability (GHSA-g94r-2vxg-569j) in
version 1.14.0. Both updates should be made in the project file to ensure all
transitive and direct dependencies are patched.
In `@src/Altinn.Correspondence.API/Caching/EmbeddedGarnetServer.cs`:
- Around line 42-43: The code does not validate that the EndPoints collection
from parsed ConfigurationOptions contains elements before calling
FirstOrDefault(), which returns null when the collection is empty and masks
misconfiguration. Add validation to check that options.EndPoints is not empty
after parsing the redisConnectionString, and throw or log an appropriate error
if no endpoints are found instead of allowing the null value to silently pass to
ResolveEndpoint. This ensures invalid or empty connection strings fail
explicitly rather than falling back to hardcoded defaults without warning.
- Line 82: The IsStartupProbeException method currently only checks for
RedisConnectionException, RedisTimeoutException, and RedisServerException, but
other transient exceptions like SocketException, IOException, and
TimeoutException can occur during startup when the TCP listener is not yet
ready. Expand the IsStartupProbeException method to also include checks for
these additional exception types so they are properly caught and retried by the
exception filter on line 82 instead of causing immediate failure.
- Line 28: Add parameter validation to the EnsureStarted method to ensure the
redisConnectionString parameter is neither null nor empty before it is used. At
the beginning of the method, before any other logic, check that
redisConnectionString meets these requirements and throw an appropriate
exception (such as ArgumentException or ArgumentNullException) if validation
fails, providing a meaningful error message to help with debugging.
- Around line 61-64: The GarnetServer instance created in the initialization
block is started with server.Start() but not assigned to the _server field until
after WaitUntilAcceptingConnections() is called. If the readiness probe throws
an exception, the server assignment never occurs, leaving the started server
instance unreachable and preventing proper cleanup or disposal. Fix this by
assigning _server = server before calling WaitUntilAcceptingConnections(), so
that any exception during the readiness probe will still allow the server to be
properly stopped and disposed through _server, rather than leaving it running
with the port bound.
- Around line 21-68: The EmbeddedGarnetServer class creates a GarnetServer
instance and stores it in the static _server field, but never disposes it when
the application shuts down, causing resource leaks. Create an IHostedService
implementation that calls _server?.Dispose() in its StopAsync method to ensure
proper cleanup of network handlers and background tasks when the application
stops. Register this service in the dependency injection container so it gets
invoked during application shutdown.
In `@src/Altinn.Correspondence.API/Program.cs`:
- Around line 134-139: The `generalSettings` object may be null if the
configuration section binding fails, which will cause a NullReferenceException
when accessing `generalSettings.RedisConnectionString` in the
`EmbeddedGarnetServer.EnsureStarted()` call. Add a null check for
`generalSettings` before the IsDevelopment() block (or within it) to validate
that the configuration was bound successfully. If it is null, log an error using
`bootstrapLogger` and either throw an exception or skip the Garnet
initialization with an appropriate warning message to ensure the application
startup fails gracefully with a clear error message.
🪄 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: 0ed1595e-eded-4f55-944e-4d65810dcb12
📒 Files selected for processing (4)
docker-compose.ymlsrc/Altinn.Correspondence.API/Altinn.Correspondence.API.csprojsrc/Altinn.Correspondence.API/Caching/EmbeddedGarnetServer.cssrc/Altinn.Correspondence.API/Program.cs
| </PackageReference> | ||
| <PackageReference Include="StackExchange.Redis" Version="2.9.32" /> | ||
| <PackageReference Include="Microsoft.Extensions.Caching.StackExchangeRedis" Version="10.0.0" /> | ||
| <PackageReference Include="Microsoft.Garnet" Version="1.1.10" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Identify Snappier transitive dependency and check for Garnet updates.
# Check which package references Snappier
dotnet list src/Altinn.Correspondence.API/Altinn.Correspondence.API.csproj package --include-transitive | grep -i snappier -B 5
# Check latest Microsoft.Garnet version
echo "Checking latest Microsoft.Garnet version..."
dotnet list src/Altinn.Correspondence.API/Altinn.Correspondence.API.csproj package --outdated | grep -i garnetRepository: Altinn/altinn-correspondence
Length of output: 224
🌐 Web query:
Microsoft.Garnet latest version and Snappier dependency version
💡 Result:
As of June 15, 2026, the latest stable version of Microsoft Garnet is v1.1.10, released on May 28, 2026 [1]. There is also a newer preview version, v2.0.1-beta.3, which was released on June 9, 2026 [2]. Regarding the Snappier dependency, while Garnet's centralized dependency file (Directory.Packages.props) does not list Snappier directly in its public repository main branch [3], Snappier has been identified as a dependency often used within the.NET ecosystem [4][5]. The Snappier package version 1.3.1 was released to address a high-severity vulnerability (GHSA-pggp-6c3x-2xmx) that affected all versions up to and including 1.3.0 [4][5]. If your Garnet project relies on Snappier, it is recommended to ensure you are using version 1.3.1 or higher to mitigate this security issue [4][6][5].
Citations:
- 1: https://github.com/microsoft/garnet/releases/tag/v1.1.10
- 2: https://newreleases.io/project/github/microsoft/garnet/release/v2.0.1-beta.3
- 3: https://github.com/microsoft/garnet/blob/main/Directory.Packages.props
- 4: .Net: Bump Snappier to 1.3.1 to fix NU1903 high-severity vulnerability (GHSA-pggp-6c3x-2xmx) microsoft/semantic-kernel#13960
- 5: https://osv.dev/vulnerability/GHSA-pggp-6c3x-2xmx
- 6: microsoft/semantic-kernel@fb10d92
🌐 Web query:
GHSA-pggp-6c3x-2xmx Snappier vulnerability details and patched version
💡 Result:
The vulnerability GHSA-pggp-6c3x-2xmx (CVE-2026-44302) in the Snappier library involves an uncatchable infinite loop that occurs when the SnappyStream component attempts to decompress malformed framed-format Snappy input as small as 15 bytes [1][2][3]. This loop consumes CPU resources, causing a denial-of-service condition that cannot be handled via standard try/catch blocks because no exception is thrown [1][3]. The issue affects all versions of Snappier up to and including version 1.3.0 [1][4]. It has been patched in version 1.3.1 [5][3][4]. Users are advised to upgrade to version 1.3.1 or later to resolve this vulnerability [3].
Citations:
- 1: https://osv.dev/vulnerability/GHSA-pggp-6c3x-2xmx
- 2: GHSA-pggp-6c3x-2xmx
- 3: https://app.opencve.io/cve/CVE-2026-44302
- 4: GHSA-pggp-6c3x-2xmx
- 5: https://nvd.nist.gov/view/vuln/detail?vulnId=CVE-2026-44302
🌐 Web query:
GHSA-g94r-2vxg-569j OpenTelemetry.Api vulnerability details and patched version
💡 Result:
The vulnerability GHSA-g94r-2vxg-569j (CVE-2026-40894) is a memory exhaustion issue (CWE-789) affecting the OpenTelemetry.NET ecosystem [1][2]. Details: The vulnerability exists in the baggage, B3, and Jaeger propagation header processing logic within the OpenTelemetry.Api and OpenTelemetry.Extensions.Propagators NuGet packages [1][3]. Specifically, these libraries were found to eagerly allocate intermediate arrays when parsing propagation headers, particularly when dealing with degenerate or malformed inputs containing large numbers of delimiter characters [1]. An attacker can exploit this by sending specially crafted, excessively large propagation headers, leading to excessive memory allocation and a potential denial of service (DoS) in the consuming application [1][4][3]. Affected Versions: - OpenTelemetry.Api: versions 0.5.0-beta.2 through 1.15.2 [1][2] - OpenTelemetry.Extensions.Propagators: versions 1.3.1 through 1.15.2 [2][3] Patched Version: The issue was addressed in version 1.15.3 [1][2][3]. Users are advised to upgrade to version 1.15.3 or higher [4][3]. Workarounds: If an immediate upgrade is not possible, the vulnerability can be mitigated by configuring appropriate HTTP request header size limits at the infrastructure level (e.g., in IIS or Nginx) or by disabling baggage and/or trace propagation [1][4].
Citations:
- 1: GHSA-g94r-2vxg-569j
- 2: https://nvd.nist.gov/vuln/detail/CVE-2026-40894
- 3: https://app.opencve.io/cve/CVE-2026-40894
- 4: https://security.snyk.io/vuln/SNYK-DOTNET-OPENTELEMETRYAPI-16189068
Update package versions to address high-severity Snappier and moderate-severity OpenTelemetry.Api vulnerabilities.
Microsoft.Garnet 1.1.10 likely brings Snappier as a transitive dependency. The Snappier vulnerability (GHSA-pggp-6c3x-2xmx) affects versions up to 1.3.0 and is patched in 1.3.1. Additionally, OpenTelemetry.Api 1.14.0 has a memory exhaustion vulnerability (GHSA-g94r-2vxg-569j) that should be upgraded to 1.15.3 or higher.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Altinn.Correspondence.API/Altinn.Correspondence.API.csproj` at line 37,
Update the PackageReference for Microsoft.Garnet to a version that brings
Snappier 1.3.1 or higher to address the high-severity Snappier vulnerability
(GHSA-pggp-6c3x-2xmx). Additionally, locate and update the OpenTelemetry.Api
PackageReference to version 1.15.3 or higher to resolve the moderate-severity
memory exhaustion vulnerability (GHSA-g94r-2vxg-569j) in version 1.14.0. Both
updates should be made in the project file to ensure all transitive and direct
dependencies are patched.
| internal static class EmbeddedGarnetServer | ||
| { | ||
| private static readonly object _lock = new(); | ||
| private static readonly TimeSpan StartupProbeDelay = TimeSpan.FromMilliseconds(100); | ||
| private const int MaxStartupProbeAttempts = 50; | ||
| private static GarnetServer? _server; | ||
|
|
||
| public static void EnsureStarted(string redisConnectionString, ILogger logger) | ||
| { | ||
| if (_server is not null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| lock (_lock) | ||
| { | ||
| if (_server is not null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var options = ConfigurationOptions.Parse(redisConnectionString); | ||
| var (host, port) = ResolveEndpoint(options.EndPoints.FirstOrDefault()); | ||
|
|
||
| var serverArgs = new List<string> | ||
| { | ||
| "--bind", host, | ||
| "--port", port.ToString(), | ||
| }; | ||
|
|
||
| if (!string.IsNullOrEmpty(options.Password)) | ||
| { | ||
| serverArgs.Add("--auth"); | ||
| serverArgs.Add("Password"); | ||
| serverArgs.Add("--password"); | ||
| serverArgs.Add(options.Password); | ||
| } | ||
|
|
||
| logger.LogInformation("Starting embedded Garnet server on {Host}:{Port}", host, port); | ||
|
|
||
| var server = new GarnetServer(serverArgs.ToArray()); | ||
| server.Start(); | ||
| WaitUntilAcceptingConnections(redisConnectionString); | ||
| _server = server; | ||
|
|
||
| logger.LogInformation("Embedded Garnet server started"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if GarnetServer implements IDisposable and requires cleanup.
# Search for GarnetServer class definition and IDisposable implementation
ast-grep --pattern $'class GarnetServer : $$$, IDisposable, $$$'
# Also check for Dispose method
ast-grep --pattern $'class GarnetServer {
$$$
Dispose($$$) {
$$$
}
$$$
}'Repository: Altinn/altinn-correspondence
Length of output: 54
🏁 Script executed:
# Find imports and usages of GarnetServer
rg "GarnetServer" --type cs -B 2 -A 2Repository: Altinn/altinn-correspondence
Length of output: 2861
🏁 Script executed:
# Check for project file to see Garnet package dependency
fd -e ".*proj" -type f | xargs grep -l "Garnet" 2>/dev/nullRepository: Altinn/altinn-correspondence
Length of output: 245
🏁 Script executed:
# Search for any existing disposal/shutdown patterns in the codebase
rg "IDisposable|Dispose\(\)|IHostedService" --type cs -lRepository: Altinn/altinn-correspondence
Length of output: 574
🏁 Script executed:
# Find .csproj files and search for Garnet package
find . -name "*.csproj" -type f | xargs grep -l "Garnet" 2>/dev/nullRepository: Altinn/altinn-correspondence
Length of output: 137
🏁 Script executed:
# Also check .sln and other config files
find . -maxdepth 3 -type f \( -name "*.csproj" -o -name "nuget.config" -o -name "packages.config" \) | head -20Repository: Altinn/altinn-correspondence
Length of output: 704
🏁 Script executed:
# Check the specific project file for Garnet reference
cat src/Altinn.Correspondence.API/Altinn.Correspondence.API.csproj 2>/dev/null | grep -A 5 -B 5 "Garnet"Repository: Altinn/altinn-correspondence
Length of output: 697
🌐 Web query:
GarnetServer IDisposable cleanup requirements documentation
💡 Result:
To properly clean up a GarnetServer instance, you should follow standard.NET IDisposable patterns while leveraging the server's specific graceful shutdown capabilities. Basic Disposal As GarnetServer implements IDisposable, you can manage its lifecycle using the using statement or by manually calling Dispose [1][2]. Disposing the server instance ensures that internal resources, such as network handlers, storage wrappers, and background tasks, are properly released [3][4][5]. Graceful Shutdown For a more controlled shutdown—particularly in production environments like Kubernetes—it is recommended to use the GarnetServer.ShutdownAsync method [6]. This orchestration method is designed to: 1. Stop accepting new connections (via StopListening) [6]. 2. Allow active connections to drain within a specified timeout [6]. 3. Commit final state, including AOF (Append-Only File) logging and checkpoints [6]. Implementation Example The standard pattern for hosting GarnetServer involves initializing the server and ensuring cleanup occurs upon termination signals (such as SIGINT or SIGTERM) [6][1][7]. // Conceptual example of graceful shutdown integration using var server = new GarnetServer(args); server.Start; // In a real-world scenario (e.g., service or console app), // trigger this on shutdown signals: await server.ShutdownAsync; server.Dispose; Key Considerations - Resource Cleanup: Internally, disposing of GarnetServer triggers the disposal of components like StoreWrapper, cluster providers, and task managers [3]. - Socket Handling: Recent improvements ensure that server disposal and network handler cleanup are handled more effectively, moving away from forced TCP resets toward graceful FIN-based connection termination to avoid client-side issues like SYN flooding [8][4]. - Background Tasks: Using ShutdownAsync helps prevent issues where background tasks might otherwise be abruptly cancelled, ensuring data integrity during persistence operations [6].
Citations:
- 1: https://microsoft.github.io/garnet/docs/welcome/releases
- 2: https://github.com/microsoft/garnet/blob/main/website/docs/welcome/releases.md
- 3: https://github.com/microsoft/garnet/blob/main/libs/server/StoreWrapper.cs
- 4: Improve Network Resource Cleanup on TLS Disconnect/Fail Paths microsoft/garnet#1694
- 5: Fix Server Exit Null Reference Exception microsoft/garnet#747
- 6: Implement graceful shutdown for Garnet server ( #1382 ) microsoft/garnet#1551
- 7: https://github.com/microsoft/garnet/blob/main/main/GarnetServer/Program.cs
- 8: Gracefully terminating TCP connections during server.Dispose microsoft/garnet#1051
🏁 Script executed:
# Read Program.cs to understand the app initialization pattern
cat src/Altinn.Correspondence.API/Program.csRepository: Altinn/altinn-correspondence
Length of output: 9080
🏁 Script executed:
# Search for IHostedService implementations in the codebase
rg "IHostedService|BackgroundService" --type csRepository: Altinn/altinn-correspondence
Length of output: 685
Add IHostedService to dispose of GarnetServer on app shutdown.
GarnetServer implements IDisposable and requires cleanup of network handlers, storage wrappers, and background tasks on shutdown. The current code creates the singleton but never disposes it, which prevents graceful cleanup in test scenarios and graceful application shutdown.
Create an IHostedService (similar to the existing EdDsaSecurityKeysCacheService pattern) to call _server?.Dispose() when the application stops, or register a shutdown callback with the host.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Altinn.Correspondence.API/Caching/EmbeddedGarnetServer.cs` around lines
21 - 68, The EmbeddedGarnetServer class creates a GarnetServer instance and
stores it in the static _server field, but never disposes it when the
application shuts down, causing resource leaks. Create an IHostedService
implementation that calls _server?.Dispose() in its StopAsync method to ensure
proper cleanup of network handlers and background tasks when the application
stops. Register this service in the dependency injection container so it gets
invoked during application shutdown.
| private const int MaxStartupProbeAttempts = 50; | ||
| private static GarnetServer? _server; | ||
|
|
||
| public static void EnsureStarted(string redisConnectionString, ILogger logger) |
There was a problem hiding this comment.
Validate redisConnectionString parameter.
The method does not validate that redisConnectionString is non-null and non-empty before passing it to ConfigurationOptions.Parse. A null or empty string could cause ArgumentException or unexpected parse results.
🛡️ Proposed fix to add validation
public static void EnsureStarted(string redisConnectionString, ILogger logger)
{
+ if (string.IsNullOrWhiteSpace(redisConnectionString))
+ {
+ throw new ArgumentException("Redis connection string cannot be null or empty.", nameof(redisConnectionString));
+ }
+
if (_server is not null)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Altinn.Correspondence.API/Caching/EmbeddedGarnetServer.cs` at line 28,
Add parameter validation to the EnsureStarted method to ensure the
redisConnectionString parameter is neither null nor empty before it is used. At
the beginning of the method, before any other logic, check that
redisConnectionString meets these requirements and throw an appropriate
exception (such as ArgumentException or ArgumentNullException) if validation
fails, providing a meaningful error message to help with debugging.
| var options = ConfigurationOptions.Parse(redisConnectionString); | ||
| var (host, port) = ResolveEndpoint(options.EndPoints.FirstOrDefault()); |
There was a problem hiding this comment.
Null-reference hazard: EndPoints collection may be empty.
options.EndPoints.FirstOrDefault() returns null if the collection is empty. The downstream ResolveEndpoint has a fallback, but this silently masks misconfiguration. If the connection string is invalid or contains no endpoints, the server binds to a hardcoded default (127.0.0.1:6379) without warning.
🛡️ Proposed fix to validate endpoints
var options = ConfigurationOptions.Parse(redisConnectionString);
- var (host, port) = ResolveEndpoint(options.EndPoints.FirstOrDefault());
+ var endpoint = options.EndPoints.FirstOrDefault();
+ if (endpoint is null)
+ {
+ throw new InvalidOperationException($"Redis connection string '{redisConnectionString}' contains no endpoints.");
+ }
+ var (host, port) = ResolveEndpoint(endpoint);📝 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.
| var options = ConfigurationOptions.Parse(redisConnectionString); | |
| var (host, port) = ResolveEndpoint(options.EndPoints.FirstOrDefault()); | |
| var options = ConfigurationOptions.Parse(redisConnectionString); | |
| var endpoint = options.EndPoints.FirstOrDefault(); | |
| if (endpoint is null) | |
| { | |
| throw new InvalidOperationException($"Redis connection string '{redisConnectionString}' contains no endpoints."); | |
| } | |
| var (host, port) = ResolveEndpoint(endpoint); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Altinn.Correspondence.API/Caching/EmbeddedGarnetServer.cs` around lines
42 - 43, The code does not validate that the EndPoints collection from parsed
ConfigurationOptions contains elements before calling FirstOrDefault(), which
returns null when the collection is empty and masks misconfiguration. Add
validation to check that options.EndPoints is not empty after parsing the
redisConnectionString, and throw or log an appropriate error if no endpoints are
found instead of allowing the null value to silently pass to ResolveEndpoint.
This ensures invalid or empty connection strings fail explicitly rather than
falling back to hardcoded defaults without warning.
| var server = new GarnetServer(serverArgs.ToArray()); | ||
| server.Start(); | ||
| WaitUntilAcceptingConnections(redisConnectionString); | ||
| _server = server; |
There was a problem hiding this comment.
Resource leak: GarnetServer started but not assigned if readiness probe fails.
server.Start() is called on line 62, but _server = server assignment happens on line 64, after WaitUntilAcceptingConnections. If the readiness probe throws, the lock is released but the started server instance is lost, with no way to stop or dispose it. This leaves the server running and the TCP port bound.
🔒 Proposed fix to assign before probing or wrap in try-catch
Option 1: Assign _server before probing so cleanup is possible on failure:
var server = new GarnetServer(serverArgs.ToArray());
server.Start();
+ _server = server;
WaitUntilAcceptingConnections(redisConnectionString);
- _server = server;Option 2: Wrap in try-catch and dispose server on probe failure:
var server = new GarnetServer(serverArgs.ToArray());
- server.Start();
- WaitUntilAcceptingConnections(redisConnectionString);
- _server = server;
+ try
+ {
+ server.Start();
+ WaitUntilAcceptingConnections(redisConnectionString);
+ _server = server;
+ }
+ catch
+ {
+ server.Dispose();
+ throw;
+ }Note: Verify that GarnetServer implements IDisposable for option 2.
📝 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.
| var server = new GarnetServer(serverArgs.ToArray()); | |
| server.Start(); | |
| WaitUntilAcceptingConnections(redisConnectionString); | |
| _server = server; | |
| var server = new GarnetServer(serverArgs.ToArray()); | |
| server.Start(); | |
| _server = server; | |
| WaitUntilAcceptingConnections(redisConnectionString); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Altinn.Correspondence.API/Caching/EmbeddedGarnetServer.cs` around lines
61 - 64, The GarnetServer instance created in the initialization block is
started with server.Start() but not assigned to the _server field until after
WaitUntilAcceptingConnections() is called. If the readiness probe throws an
exception, the server assignment never occurs, leaving the started server
instance unreachable and preventing proper cleanup or disposal. Fix this by
assigning _server = server before calling WaitUntilAcceptingConnections(), so
that any exception during the readiness probe will still allow the server to be
properly stopped and disposed through _server, rather than leaving it running
with the port bound.
| connection.GetDatabase().Ping(); | ||
| return; | ||
| } | ||
| catch (Exception ex) when (IsStartupProbeException(ex)) |
There was a problem hiding this comment.
Startup probe exception filter may be incomplete.
The filter on line 82 only retries RedisConnectionException, RedisTimeoutException, and RedisServerException. Other transient exceptions like SocketException, IOException, or TimeoutException will cause immediate failure without retries. This could lead to false negatives if the server is starting but the TCP listener is not yet ready.
🛡️ Proposed fix to widen exception filter
private static bool IsStartupProbeException(Exception exception)
{
- return exception is RedisConnectionException or RedisTimeoutException or RedisServerException;
+ return exception is RedisConnectionException
+ or RedisTimeoutException
+ or RedisServerException
+ or SocketException
+ or IOException
+ or TimeoutException;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Altinn.Correspondence.API/Caching/EmbeddedGarnetServer.cs` at line 82,
The IsStartupProbeException method currently only checks for
RedisConnectionException, RedisTimeoutException, and RedisServerException, but
other transient exceptions like SocketException, IOException, and
TimeoutException can occur during startup when the TCP listener is not yet
ready. Expand the IsStartupProbeException method to also include checks for
these additional exception types so they are properly caught and retried by the
exception filter on line 82 instead of causing immediate failure.
| if (hostEnvironment.IsDevelopment()) | ||
| { | ||
| // Run Garnet in-process for local development and tests instead of a Docker Redis container. | ||
| // The StackExchange.Redis clients below connect to it via RedisConnectionString unchanged. | ||
| EmbeddedGarnetServer.EnsureStarted(generalSettings.RedisConnectionString, bootstrapLogger); | ||
| } |
There was a problem hiding this comment.
Null-reference hazard: generalSettings may be null.
Line 133 calls config.GetSection(nameof(GeneralSettings)).Bind(generalSettings), but if the configuration section is missing or binding fails, generalSettings.RedisConnectionString on line 138 will throw NullReferenceException. The app startup will crash with an unclear error.
🛡️ Proposed fix to validate configuration
var generalSettings = new GeneralSettings();
config.GetSection(nameof(GeneralSettings)).Bind(generalSettings);
+ if (string.IsNullOrWhiteSpace(generalSettings.RedisConnectionString))
+ {
+ throw new InvalidOperationException("GeneralSettings:RedisConnectionString is required but was not found in configuration.");
+ }
if (hostEnvironment.IsDevelopment())
{
// Run Garnet in-process for local development and tests instead of a Docker Redis container.
// The StackExchange.Redis clients below connect to it via RedisConnectionString unchanged.
EmbeddedGarnetServer.EnsureStarted(generalSettings.RedisConnectionString, bootstrapLogger);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Altinn.Correspondence.API/Program.cs` around lines 134 - 139, The
`generalSettings` object may be null if the configuration section binding fails,
which will cause a NullReferenceException when accessing
`generalSettings.RedisConnectionString` in the
`EmbeddedGarnetServer.EnsureStarted()` call. Add a null check for
`generalSettings` before the IsDevelopment() block (or within it) to validate
that the configuration was bound successfully. If it is null, log an error using
`bootstrapLogger` and either throw an exception or skip the Garnet
initialization with an appropriate warning message to ensure the application
startup fails gracefully with a clear error message.
1 participant
Description
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit