Skip to content

Downgrade 4 ATM user token log messages from Error to Warning - #317

Merged
Erwinvandervalk merged 2 commits into
mainfrom
dh/downgrade-atm-log-levels
Feb 19, 2026
Merged

Downgrade 4 ATM user token log messages from Error to Warning#317
Erwinvandervalk merged 2 commits into
mainfrom
dh/downgrade-atm-log-levels

Conversation

@damianh

@damianh damianh commented Feb 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

Downgrades 4 log messages in Duende.AccessTokenManagement.OpenIdConnect from LogLevel.Error to LogLevel.Warning. These conditions represent normal authentication lifecycle events (expired sessions, expired refresh tokens) rather than application errors.

Related: DuendeSoftware/products-private#2505

Problem

Users report that these Error-level log messages trigger unnecessary production alerts. They cannot selectively filter them because turning off all errors in the Duende namespace would also hide genuine errors.

Changes

File Log Method Rationale
UserAccessTokenManagementService.cs:39 CannotRetrieveAccessTokenDueToNoActiveUser Fires when user.Identity.IsAuthenticated is false — expected when session expires
StoreTokensInAuthenticationProperties.cs:53 FailedToGetUserTokenDueToMissingTokensInCookie Commonly occurs due to expired sessions that resolve via login redirect
OpenIdConnectUserTokenEndpoint.cs:132 FailedToRefreshAccessToken Refresh token expiration/revocation is part of normal token lifecycle
OpenIdConnectUserTokenEndpoint.cs:212 FailedToRevokeAccessToken Revocation is best-effort; failure doesn't propagate and tokens expire naturally

What stays at Error

FailedToRequestAccessTokenForClient (client credentials) remains at LogLevel.Error — client credentials failures are not related to user session lifecycle and could indicate genuine infrastructure problems.

Consistency

This also resolves a log level inconsistency in the pipeline:

  • Before: Information → Error → Warning
  • After: Information → Warning → Warning

Copilot AI review requested due to automatic review settings February 12, 2026 16:46
Copilot AI reviewed Feb 12, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Downgrades several user-token lifecycle log events in Duende.AccessTokenManagement.OpenIdConnect from LogLevel.Error to LogLevel.Warning to avoid production alert noise for expected session/refresh-token scenarios.

Changes:

  • Downgraded “no active user” log from Error to Warning when the user is unauthenticated.
  • Downgraded “missing tokens in cookie/properties” log from Error to Warning.
  • Downgraded refresh-token refresh/revoke failure logs from Error to Warning.

Reviewed changes

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

File Description
access-token-management/src/AccessTokenManagement.OpenIdConnect/Internal/UserAccessTokenManagementService.cs Downgrades unauthenticated-user access token retrieval log to Warning.
access-token-management/src/AccessTokenManagement.OpenIdConnect/Internal/StoreTokensInAuthenticationProperties.cs Downgrades missing-cookie-token log to Warning for common session-expiry scenarios.
access-token-management/src/AccessTokenManagement.OpenIdConnect/Internal/OpenIdConnectUserTokenEndpoint.cs Downgrades refresh/revoke error logs to Warning for expected token lifecycle outcomes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@damianh damianh self-assigned this Feb 12, 2026
@damianh damianh mentioned this pull request Feb 12, 2026
Open
@damianh damianh linked an issue Feb 12, 2026 that may be closed by this pull request
Log severity is maybe too high #311
Open
@josephdecock

josephdecock commented Feb 13, 2026

Copy link
Copy Markdown
Member

I think some of these are expected and should go down to info level.

pgermishuys
pgermishuys previously approved these changes Feb 13, 2026
View reviewed changes

@pgermishuys pgermishuys left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These downgrading of log messages does make sense. None of the messages that were previously error messages would require or warrant an alert being raised and would likely just lead to Alert fatigue.

I do however agree with @josephdecock. We could we revise these and determine whether we should further downgrade them from Warning to Information.

@pgermishuys
pgermishuys self-requested a review February 13, 2026 14:08
@damianh damianh assigned pgermishuys and unassigned damianh Feb 13, 2026
@pgermishuys
pgermishuys force-pushed the dh/downgrade-atm-log-levels branch from 2d00223 to e58b816 Compare February 13, 2026 15:24
@adamralph

adamralph commented Feb 13, 2026

Copy link
Copy Markdown
Member

Given "These conditions represent normal authentication lifecycle events" I'm also in support of downgrading to info. That was my initial reaction when I first saw this, before I saw the comments above.

@damianh

damianh commented Feb 16, 2026

Copy link
Copy Markdown
Member Author

Will need a second PR targetting atm/releases/4.1.x

@Erwinvandervalk

Erwinvandervalk commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

Suggestion: Split log levels — 2 to Information, 2 stay at Warning

After reviewing the four log messages, I think we should differentiate based on whether the failure involves an external call:

Downgrade to Information (purely local checks)

  • CannotRetrieveAccessTokenDueToNoActiveUser — This is a local IsAuthenticated check. An unauthenticated user is a normal, expected condition during session expiry. No external call is made, nothing failed — we're just observing the current state.
  • FailedToGetUserTokenDueToMissingTokensInCookie — Same reasoning. No tokens in the cookie is expected when sessions expire. This is resolved by a login redirect. Purely a local observation.

Keep at Warning (failed external calls)

  • FailedToRefreshAccessToken — While individual refresh token expiration is normal, this involves a failed call to the token endpoint. A spike in these could indicate the identity provider is down or misconfigured. Warning makes these easy to aggregate in dashboards for pattern detection without triggering per-occurrence alerts.
  • FailedToRevokeAccessToken — Similarly, this is a failed HTTP call to the revocation endpoint. While revocation is best-effort and failures don't propagate, operators should still be able to spot trends (e.g., revocation endpoint unreachable).

This gives operators the right signal: Information for expected local state, Warning for external call failures that are individually tolerable but worth monitoring in aggregate.

damianh and others added 2 commits February 19, 2026 11:28
@damianh @Erwinvandervalk
Downgrade 4 user token log messages from Error to Warning
6cce060 
These conditions represent normal authentication lifecycle events (expired
sessions, expired refresh tokens) rather than application errors. Logging
them at Error triggers unnecessary production alerts that users cannot
selectively filter without also suppressing genuine errors.

Resolves DuendeSoftware/issues#1229
@Erwinvandervalk
Downgrade local-check log messages from Warning to Information
90e9ce9 
CannotRetrieveAccessTokenDueToNoActiveUser and
FailedToGetUserTokenDueToMissingTokensInCookie are purely local state
checks (unauthenticated user, missing cookie tokens) that are expected
during normal session expiry. Information is the appropriate level.

FailedToRefreshAccessToken and FailedToRevokeAccessToken remain at
Warning since they represent failed external HTTP calls where a spike
could indicate infrastructure problems.
@Erwinvandervalk
Erwinvandervalk force-pushed the dh/downgrade-atm-log-levels branch from be46b2d to 90e9ce9 Compare February 19, 2026 10:29
@Erwinvandervalk
Erwinvandervalk merged commit c3b1d09 into main Feb 19, 2026
8 checks passed
@Erwinvandervalk
Erwinvandervalk deleted the dh/downgrade-atm-log-levels branch February 19, 2026 10:37
@alerchedahl

alerchedahl commented May 18, 2026

Copy link
Copy Markdown

I don't see these changes in the 4.2.0 release from Mar 18, when can we expect this in a Nuget release?
Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments
@Erwinvandervalk Erwinvandervalk Erwinvandervalk approved these changes
@pgermishuys pgermishuys Awaiting requested review from pgermishuys

Assignees

@Erwinvandervalk Erwinvandervalk

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Log severity is maybe too high

7 participants

@damianh @josephdecock @adamralph @Erwinvandervalk @alerchedahl @pgermishuys