Skip to content

perf(auth): debounce access token "last used" writes - #39175

Open
Jason4869 wants to merge 1 commit into
go-gitea:mainfrom
Jason4869:debounce-access-token-last-used
Open

perf(auth): debounce access token "last used" writes#39175
Jason4869 wants to merge 1 commit into
go-gitea:mainfrom
Jason4869:debounce-access-token-last-used

Conversation

@Jason4869

Copy link
Copy Markdown

Fixes #39174.

Every successful token authentication performs a full-row UPDATE of the token's own access_token row, purely to advance updated_unix ("last used"). UpdateAccessToken uses AllCols(), so all eight columns are rewritten, and it is called synchronously in the request path from services/auth/basic.go (PAT via Basic/Bearer/?token=) and services/auth/oauth2.go (legacy non-JWT SHA tokens), with no condition, debounce or sampling on either path.

The consequence is that every concurrent request presenting the same token serialises on that one row's exclusive lock. While the database is healthy the lock is held for well under a millisecond and this is invisible, but the per-row ceiling degrades in proportion to database latency: once it falls below the request arrival rate the queue grows without bound, so a modest database slowdown turns into a full serialisation collapse rather than proportional degradation. #39174 has the measurements and the incident this caused on our instance.

The change

A staleness guard, and both call sites wrapped in it:

const AccessTokenUseInterval = 30 * time.Second

func ShouldPersistTokenUse(last timeutil.TimeStamp, now time.Time) bool {
	return now.Sub(last.AsTime()) >= AccessTokenUseInterval
}

This is a deliberate mirror of RunnerHeartbeatInterval / ShouldPersistLastOnline, added in #38281 for the equivalent per-poll write on action_runner.last_onlinesame naming, same 30s interval, same call-site guard placement. The intent is that this reads as an extension of a pattern already accepted here rather than a novel proposal; if you would prefer a different interval, a shared constant across both tables, or the guard pushed down into UpdateAccessToken itself, I am happy to rework it.

Why this is safe

  • updated_unix is display-only. It feeds the "last used" column in the UI and the 7-day HasRecentActivity flag set in AfterLoad(). It is never consulted for an authorization decision, so bounding its freshness to 30s is not security-relevant. The worst observable effect is a token's "last used" reading up to 30s stale — well inside the 7-day window, which is the only thing besides display that reads the column.
  • Correct across multiple Gitea processes. GetAccessTokenBySHA re-reads the row from the database on every call — even on a token-cache hit, where the cache stores only the row ID and the row itself is re-fetched via db.GetEngine(ctx).ID(cached.TokenID).Get(accessToken). So the UpdatedUnix the guard compares against is always the committed value, never a per-process cached one.
  • Not a change to token validation. Only the write is skipped; lookup, hash comparison, scope and user resolution are untouched.
  • Other writers are unaffected. Editing a token's scope in the UI still goes through UpdateAccessToken unguarded, and the column carries xorm's updated tag, so those writes still advance the timestamp as before.

Testing

TestShouldPersistTokenUse added in models/auth/access_token_test.go, table-driven in the same shape as the existing TestShouldPersistLastOnline (fresh / exactly-at-interval / stale / never-used).

$ go build ./...
$ go test -tags 'sqlite sqlite_unlock_notify' -count=1 ./models/auth/... ./services/auth/...
ok  	gitea.dev/models/auth	1.795s
ok  	gitea.dev/services/auth	1.460s
ok  	gitea.dev/services/auth/source	2.004s
ok  	gitea.dev/services/auth/source/oauth2	2.529s
...
$ go vet -tags 'sqlite sqlite_unlock_notify' ./models/auth/... ./services/auth/...

Full tree builds clean; gofmt clean on all four changed files.

Every successful token authentication performs a full-row UPDATE of the
token's own `access_token` row purely to advance `updated_unix`
("last used"). `UpdateAccessToken` uses `AllCols()`, so all eight columns
are rewritten, and it is called synchronously in the request path from
both `services/auth/basic.go` (PAT via Basic/Bearer/`?token=`) and
`services/auth/oauth2.go` (legacy non-JWT SHA tokens), with no condition,
debounce or sampling on either path.

The consequence is that every concurrent request presenting the *same*
token serialises on that one row's exclusive lock. While the database is
healthy the lock is held for well under a millisecond and this is
invisible, but the per-row ceiling degrades in proportion to database
latency: once it falls below the request arrival rate the queue grows
without bound, so a modest database slowdown turns into a full
serialisation collapse rather than proportional degradation.

Add a staleness guard and wrap both call sites in it:

    const AccessTokenUseInterval = 30 * time.Second
    func ShouldPersistTokenUse(last timeutil.TimeStamp, now time.Time) bool

This mirrors `RunnerHeartbeatInterval` / `ShouldPersistLastOnline`, added
in go-gitea#38281 for the equivalent per-poll write on `action_runner.last_online`
— same naming, same 30s interval, same call-site guard placement.

`updated_unix` is display-only: it feeds the "last used" column in the UI
and the 7-day `HasRecentActivity` flag set in `AfterLoad()`. It is never
consulted for an authorization decision, so bounding its freshness to 30s
is not security-relevant; the worst observable effect is a token's
"last used" reading up to 30s stale.

The guard is also safe across multiple Gitea processes:
`GetAccessTokenBySHA` re-reads the row from the database on every call —
even on a token-cache hit, where the cache stores only the row ID and the
row itself is re-fetched — so the `UpdatedUnix` being compared is always
the committed value, never a per-process cached one.

Token validation itself is unchanged; only the write is skipped. Lookup,
hash comparison, scope and user resolution are untouched.

Signed-off-by: Jason Lowe <jason4869@gmail.com>

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.

🟢 Approval recommended

The change is minimal and pattern-consistent, reduces synchronous DB writes on hot auth paths, and includes targeted unit coverage for the new guard.

Pull request overview

This PR reduces database row-lock contention caused by updating access_token.updated_unix (“last used”) on every successful token-authenticated request by adding a 30s staleness guard, mirroring the existing debounce pattern used for Actions runner heartbeats.

Changes:

  • Add AccessTokenUseInterval and ShouldPersistTokenUse to debounce “last used” persistence for access tokens.
  • Guard the synchronous UpdateAccessToken write in both token-auth call paths (basic PAT auth and legacy SHA token OAuth2 path).
  • Add unit coverage for the new staleness guard.
File summaries
File Description
services/auth/oauth2.go Wraps access-token “last used” persistence with ShouldPersistTokenUse to avoid per-request row updates.
services/auth/basic.go Applies the same debounce guard to PAT authentication and adds the needed time import.
models/auth/access_token.go Introduces AccessTokenUseInterval and ShouldPersistTokenUse (pattern-aligned with Actions runner debounce).
models/auth/access_token_test.go Adds table-driven tests for ShouldPersistTokenUse.
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread services/auth/oauth2.go
}

t.UpdatedUnix = timeutil.TimeStampNow()
if err = auth_model.UpdateAccessToken(ctx, t); err != nil {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And it seems only the updated time should be updated, the function UpdateAccessToken could be rewrite to not update other columns.

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

Labels

lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. topic/authentication

Projects

None yet

Development

Successfully merging this pull request may close these issues.

access_token.updated_unix is written on every authenticated request, serialising all requests that share a token

4 participants