fix(guests): the middleware that keeps an active reader alive had never run - #604
Merged
Conversation
…er run
GuestActivityMiddleware decided whether a request belonged to a guest by reading
context.User.FindFirst("is_guest"). This API registers no ASP.NET authentication
middleware at all — every endpoint resolves identity by hand via GetUserId — so
HttpContext.User is permanently empty, the check was always false, and the method
returned on its first line for every request it ever served. LastActiveAt was
written once per guest, at creation.
The claim was never missing: AuthService has minted is_guest into every guest
token since guest sessions shipped, and ValidateAccessToken threw it away. It now
has a sibling that returns both, with the claim name as one constant.
Damage was narrower than it looks and the doc now says so: GuestCleanupWorker also
spares any guest holding a ReadingProgress row, so anyone who actually read a book
survived regardless.
Two things beyond the identity fix: the old debounce read the user row on every
guest request to decide whether to skip the write — the read it was avoiding — and
is now an IMemoryCache entry, so the common path touches no database; and the write
is a targeted ExecuteUpdateAsync instead of loading an entity.
Verified as a side effect, not a 200 — the lesson this same middleware taught the
first time. Against a live stack: one request moves last_active_at, five more in
the debounce window move nothing, an account's row is untouched. The decision is
now a pure function with tests; the previous version had no seam at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011rgEMvYYi4Egj99dVtvm6E
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GuestActivityMiddlewareexists soGuestCleanupWorkerdoes not delete a guest who is still reading. It decided whether a request belonged to a guest by readingcontext.User.FindFirst("is_guest").This API registers no ASP.NET authentication middleware at all — by design; every endpoint resolves identity by hand through
GetUserId. SoHttpContext.Useris permanently empty, the check was always false, and the method returned on its first line for every request it ever served.LastActiveAtwas written exactly once per guest, at creation.The claim was never missing.
AuthServicehas mintedis_guestinto every guest's access token since guest sessions shipped;ValidateAccessTokenthrew it away and returned only the id. It now has a sibling —ValidateAccessTokenIdentity— returning both, with the claim name as a single constant shared by the one place that mints it and the one that reads it.What the damage actually was
Narrower than it looks, and the docs now say so rather than implying a data-loss emergency:
GuestCleanupWorker's preservation filter also spares any guest holding aReadingProgressrow, so a guest who genuinely read a book survived regardless. The real exposure was a guest who opened books and never produced a progress row — plus a pipeline carrying a slice that did nothing, beneath a paragraph inProgram.csreasoning carefully about where to place it.Two changes beyond the identity fix
SELECTonusersto decide whether to skip theUPDATE. It is now anIMemoryCacheentry, so the common path touches no database at all.ExecuteUpdateAsyncrather than loading an entity and saving it, so nothing else on the row can be written by accident.Verification — the side effect, not the 200
That is the lesson this same middleware taught the first time, so it was checked the same way. Against a live local stack:
last_active_atThe 200 is identical in all three cases — it was identical for the whole time the middleware did nothing.
The decision itself is now a pure function (
ShouldWrite) with unit tests, including the inclusive boundary where the debounce and the cache expiry meet. The previous version had no testable seam, which is part of why the defect survived review.dotnet test tests/TextStack.UnitTests— 1240 pass (5 new)dotnet test tests/TextStack.IntegrationTests— 171 pass, 48 skippeddotnet format textstack.sln --verify-no-changes— clean🤖 Generated with Claude Code
https://claude.ai/code/session_011rgEMvYYi4Egj99dVtvm6E