Request Logging: Avoid forcing a session store load when resolving the session id for logging (closes #23082)

[AndyButland]

Description

On a distributed-session setup (the session store backed by an L2 cache via IDistributedCache — e.g. load-balanced sites or a standalone L2 cache add-on), the request-logging session-id enricher caused a synchronous, blocking L2 cache round-trip on every non-static request, including anonymous renders that never otherwise use session.

Root cause

The chain:

1. Session is registered by default (AddWebComponents → AddSession).
2. UmbracoRequestLoggingMiddleware pushes HttpSessionIdEnricher into the Serilog LogContext for every non-static request.
3. When any log event is emitted, the enricher reads ISessionIdResolver.SessionId → AspNetCoreSessionManager.SessionId → httpContext.Session.Id.
4. Reading DistributedSession.Id forces Load(), which calls the synchronous IDistributedCache.Get(...) — a blocking network round-trip on a thread-pool worker, even when no session data exists.

With the in-memory store this is a cheap dictionary lookup and invisible; with a distributed store it blocks worker threads under burst load and contributes to thread-pool starvation and request timeouts.

Fix

AspNetCoreSessionManager.SessionId now only reads httpContext.Session.Id when the incoming request carries the configured session cookie. An established session always sends its cookie back (ASP.NET Core has no cookieless session mode), so the cookie's absence means there is nothing meaningful to load — and we avoid forcing the blocking store read.

Backward compatibility

There is a comment in the code to evaluate if we even need to be enriching the log with the session ID, but given we are, people could be relying on it, so we should maintain the behaviour. The only slight change, which I think is acceptable, is the very first request where the session is established won't have a request cookie and as such the session ID won't be logged.

Testing

Automated

Added a new set of unit tests in AspNetCoreSessionManagerTests that covers this change and existing functionality.

Manual testing

Verified end-to-end on the default in-memory session store using a temporary debug controller that establishes a session and logs (so the enricher fires).

using Microsoft.AspNetCore.Mvc;
using Umbraco.Cms.Core.Net;

namespace Umbraco.Cms.Web.UI.Controllers;

[ApiController]
[Route("debug/session-id-logging")]
public class DebugSessionIdLoggingController : ControllerBase
{
    private readonly ILogger<DebugSessionIdLoggingController> _logger;
    private readonly ISessionIdResolver _sessionIdResolver;

    public DebugSessionIdLoggingController(
        ILogger<DebugSessionIdLoggingController> logger,
        ISessionIdResolver sessionIdResolver)
    {
        _logger = logger;
        _sessionIdResolver = sessionIdResolver;
    }

    [HttpGet]
    public IActionResult Get()
    {
        HttpContext.Session.SetString("debug-session-id-logging", "touched");

        var sessionId = _sessionIdResolver.SessionId;
        _logger.LogInformation("Session id logging debug request handled");

        return Ok(new { sessionId });
    }
}

Hit the endpoint twice with a shared cookie jar, so the second call sends the UMB_SESSION cookie set by the first:

curl.exe -ks -c c.txt -b c.txt https://localhost:44339/debug/session-id-logging
curl.exe -ks -c c.txt -b c.txt https://localhost:44339/debug/session-id-logging

Then check umbraco/Logs/UmbracoTraceLog.*.json for the Session id logging debug request handled entries:

{"@t":"...","@mt":"Session id logging debug request handled","RequestPath":"/debug/session-id-logging","Log4NetLevel":"INFO ","HttpRequestId":"4566ed7d-ec83-4d0f-8296-083a8681bd64","HttpRequestNumber":1}
{"@t":"...","@mt":"Session id logging debug request handled","RequestPath":"/debug/session-id-logging","Log4NetLevel":"INFO ","HttpRequestId":"7817e5f9-6ffa-4c31-9fca-0d67af7b820d","HttpRequestNumber":2,"HttpSessionId":"bf0e82df-ede2-dbcc-794b-57b5379576d0"}

• Call 1 (HttpRequestNumber: 1): no HttpSessionId property — anonymous request, no inbound session cookie, store never read.
• Call 2 (HttpRequestNumber: 2): HttpSessionId present and populated — the session cookie was sent, a real session exists, and the id is logged as before.

Stashing the AspNetCoreSessionManager change and re-running makes call 1 log a non-null HttpSessionId again (the old always-read behaviour), confirming the guard is working as expected.

[claude]

Claude finished @AndyButland's task in 3m 1s —— View job

---

PR Review

Target: origin/v17/dev · Based on commit: 82e3af29

Guards AspNetCoreSessionManager.SessionId behind a session-cookie presence check to prevent a synchronous, blocking IDistributedCache.Get round-trip on every non-static request when sessions are backed by a distributed store.

• Modified public API: none — AspNetCoreSessionManager is internal sealed; ISessionIdResolver.SessionId return type (string?) is unchanged
• Other changes: SessionId now returns null (rather than the session ID from a blocking store load) for requests that carry no inbound session cookie. The first request that establishes a session therefore omits HttpSessionId from its log entry — HttpSessionIdEnricher.Enrich() already guards against null so no log pipeline change is needed.

---

Suggestions

• tests/…/AspNetCoreSessionManagerTests.cs:22: Test method names don't follow the Can_/Cannot_ PascalCase-underscore convention used in this codebase. (Inline comment posted.)

---

Approved with Suggestions for improvement

Good to go. The fix is correct: skipping Session.Id when no cookie is present avoids the blocking distributed-cache round-trip, the behavioral nuance for the first request in a session is clearly documented, and the regression test is well-constructed (the TrackingSession.IdReadCount assertion makes it hard to accidentally pass without the guard). The one suggestion above is minor style only.

[Copilot]

Pull request overview

This PR updates AspNetCoreSessionManager.SessionId to avoid forcing an ASP.NET Core session store load during request logging when the incoming request does not carry the session cookie, preventing synchronous IDistributedCache.Get(...) round-trips on anonymous/non-session requests in distributed-session setups.

Changes:

• Guard session-id resolution behind an inbound session-cookie presence check to avoid triggering Session.Id (and thus DistributedSession.Load()).
• Inject IOptions<SessionOptions> into AspNetCoreSessionManager to read the configured session cookie name.
• Add unit tests verifying "0" behavior when sessions aren’t available, null when no session cookie is present (and Session.Id is not read), and normal behavior when the cookie is present.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/Umbraco.Web.Common/AspNetCore/AspNetCoreSessionManager.cs	Avoids session store load by only reading Session.Id when the configured session cookie is present.
tests/Umbraco.Tests.UnitTests/Umbraco.Web.Common/AspNetCore/AspNetCoreSessionManagerTests.cs	Adds regression/unit coverage ensuring Session.Id isn’t read when no session cookie is present, plus baseline behaviors.

[jemayn]

This resolves the anonymous traffic case well.

However, for any user with an established session cookie (logged-in users, anything that writes session), Session.Id still forces the synchronous load per request, just for logging - so on session-heavy traffic the thread pool starvation risk under load returns. Could we log a hash of the cookie value instead of Session.Id so log enrichment never triggers a load at all?

If the session id is only there to filter requests to the same session, then a hash of the cookie should give the same amount of info without causing these extra L2 hits.

[AndyButland]

Good point @jemayn. I like your approach, but I think if we introduce this in 17 we do need to be careful about retaining the previous behaviour (just in case anyone particularly wants and expects the actual session ID in the logs).

To support that I've made session-id log enrichment is configurable rather than a single hard-coded behaviour.

New setting Umbraco:CMS:Logging:SessionIdLogging (SessionIdLoggingMode):

• SessionId (default) — logs the actual ASP.NET Core session id. Backward compatible. Still guarded so anonymous requests with no session cookie don't force a session-store load.
• None — no session-id enrichment at all.
• CookieHash — logs a one-way SHA-256 hash of the session cookie value. Gives the same per-session correlation without ever loading the session from its store, so it never incurs a distributed-cache round-trip. The raw cookie (a session bearer token) is never logged.

Default behaviour is unchanged; operators running distributed/L2-backed sessions can opt into CookieHash or None to remove the per-request blocking load.

I'm thinking that for Cloud, when load balancing is enabled, this option could be fixed to CookieHash or None via an environment variable. Is that feasible?

Manual testing completed — running the local dev site on the default in-memory session store, two requests per mode with a shared cookie jar, HttpSessionId checked in the JSON trace log:

Mode	Call 1 (no cookie)	Call 2 (cookie)	HttpSessionId in log
SessionId	sessionId: null	sessionId: <guid>	call 1 absent; call 2 = the GUID
None	sessionId: null	sessionId: null	absent on both calls
CookieHash	sessionId: null	sessionId: <64-char hex>	call 1 absent; call 2 = the hash

Also covered by unit tests in AspNetCoreSessionManagerTests, including assertions that Session.Id is never read in None/CookieHash modes.