fix(webui): transient 503s kick authenticated users to the login page - #119
fix(webui): transient 503s kick authenticated users to the login page#119SoulKyu wants to merge 3 commits into
Conversation
SoulKyu
left a comment
There was a problem hiding this comment.
@SoulKyu Reviewed the fix in internal/webui/templates/scripts/dashboard_core.templ: handleAuthError() now redirects to /login only on response.status === 401, dropping the || response.status === 503 branch. I verified this makes the helper consistent with installFetchInterceptor(), which already only redirects on 401, and confirmed via grep that handleAuthError() is the single choke point used by every dashboard call site (actions, data, modal, resolved-alerts, utilities mixins) — so this one-line change covers all of them as claimed. The regenerated dashboard_core_templ.go matches the .templ source (only the embedded script string changed), and go build ./internal/webui/... succeeds cleanly. Nice, well-scoped fix for a real production annoyance — thanks for tracking down the root cause and keeping the diff minimal!
🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.
🧪 QA ReportEnv:
Why criterion 2 fails (for the fixer)The 503 removal is correct but insufficient: with the backend down the protected routes answer 401, not 503.
Reproducible without a browser: with the backend stopped, Exploratory: on a healthy stack nothing adjacent regressed — login → 12 alerts rendered, alert modal opens and deep-links ( Verdict: FAIL — 1 of 3 acceptance criteria fails. Fixer summoned. |
bc738d6 to
b134c8c
Compare
|
🔀 Rebase (conflicts resolved): Rebased onto Conflict: Resolution: discarded the conflicted generated hunk and re-ran Semantic outcome preserved: the auth guard in Validation: |
SoulKyu
left a comment
There was a problem hiding this comment.
@SoulKyu Thanks for tracking this down — restricting handleAuthError() to response.status === 401 correctly stops the cache-warm-up 503 from bouncing users to /login, and the regenerated dashboard_core_templ.go matches the .templ diff.
However, per the QA run already recorded on this PR and confirmed below by re-reading the auth path, the change doesn't close the second scenario the PR body itself calls out ("Backend blips hit users mid-incident"). See the inline comment for details and a concrete fix direction. (Posted as COMMENT because GitHub blocks self-authored REQUEST_CHANGES reviews; this is a blocking finding and should not be merged as-is.)
🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.
|
Looper fixer round complete — 3899f87
🔁 Powered by Looper · runner=fixer · agent=claude-code · An autonomous AI dev team for your GitHub repos. |
🧪 QA ReportRe-test after the conflict rebase (head
Why criterion 2 still fails (for the fixer)Unchanged from the previous head — the rebase did not touch this. With the backend down, the protected routes answer
Reproducible without a browser (same session cookie throughout): The 401 is caused solely by the backend being unreachable. Fixing this means making Exploratory: no adjacent regression on a healthy stack — login renders alerts, search Verdict: FAIL — 1 of 3 acceptance criteria fails. Fixer summoned. |
|
🔀 Rebased on |
3899f87 to
d120840
Compare
SoulKyu
left a comment
There was a problem hiding this comment.
@SoulKyu Verified the fix at head d120840. The remaining blocking gap from the earlier review round — protected routes answering 401 (with ClearSession) instead of 503 when the backend is merely unreachable — is now properly closed:
internal/webui/client/backend_client.go: newIsUnavailableError()unwraps the gRPC status and returns true only forcodes.Unavailable/codes.DeadlineExceeded, leaving genuine session-rejection errors (e.g.codes.Unauthenticated, plain non-gRPC errors) to fall through as real auth failures.internal/webui/middleware/auth.goRequireAuth(): whenValidateSessionfails with an unavailable-backend error, it now responds503and returns without callingClearSession, reserving401+ session-clear for actual invalid/expired sessions.internal/webui/templates/scripts/dashboard_core.templ:handleAuthError()redirects to/loginonly on401, matching the unchangedinstallFetchInterceptor()— confirmed via grep that both are now consistent (response.status === 401in both places, no remaining503redirect condition).- The regenerated
dashboard_core_templ.godiff is a single-line change matching the.templsource, and the newTestIsUnavailableErrortable test covers nil,Unavailable,DeadlineExceeded, a plain non-gRPC error, andUnauthenticated, exercising exactly the classification the fix relies on.
Locally re-verified: go build ./..., go vet ./internal/webui/..., and go test ./internal/webui/client/... ./internal/webui/middleware/... all pass. This closes both scenarios from #118 (cache warm-up and backend-down mid-incident) without touching unrelated call sites. Solid follow-through on the QA feedback — thanks for tracking the root cause all the way through the gRPC error path instead of settling for the surface-level frontend fix.
🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.
🧪 QA ReportEnv:
Exploratory: happy path with everything up is unaffected — Verdict: PASS |
|
🔀 Rebased on |
d120840 to
1e22b58
Compare
SoulKyu
left a comment
There was a problem hiding this comment.
@SoulKyu Verified at head 1e22b58. The webui-relevant files in this PR (internal/webui/client/backend_client.go, internal/webui/client/backend_client_test.go, internal/webui/middleware/auth.go, internal/webui/templates/scripts/dashboard_core.templ + regenerated _templ.go) are byte-identical to the commit already reviewed and approved as clean in the prior round (d120840) — the only change since then is an unrelated devtools/factory-tui documentation update outside this PR's scope, confirmed via gh pr diff --name-only against the PR's actual file list.
Re-confirmed independently:
client.IsUnavailableError()correctly unwraps the gRPC status and only returns true forcodes.Unavailable/codes.DeadlineExceeded, leavingcodes.Unauthenticatedand plain non-gRPC errors (e.g.invalid sessionfromValidateSession) to fall through as real auth failures.middleware.RequireAuth()now returns 503 without clearing the session when the backend is merely unreachable, and only clears the session + returns 401 for genuine invalid/expired sessions.handleAuthError()indashboard_core.templredirects to/loginonly on 401, matching the unchangedinstallFetchInterceptor().TestIsUnavailableErrorcovers nil, Unavailable, DeadlineExceeded, a plain error, and Unauthenticated.go build ./internal/webui/...,go vet ./internal/webui/..., andgo test ./internal/webui/client/... ./internal/webui/middleware/...all pass at this head.
This closes both scenarios from #118 (cache warm-up and backend-down mid-incident). Thanks for the thorough follow-through tracking this all the way to the gRPC error classification instead of stopping at the frontend symptom!
🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.
|
🔀 Rebased on |
1e22b58 to
5bedaa1
Compare
SoulKyu
left a comment
There was a problem hiding this comment.
@SoulKyu Verified at head 5bedaa1. This head carries no code changes versus the previously-approved head (1e22b58) — a diff between the two shows zero differences in internal/webui/client/backend_client.go, internal/webui/client/backend_client_test.go, internal/webui/middleware/auth.go, internal/webui/templates/scripts/dashboard_core.templ, and the regenerated dashboard_core_templ.go; only the commit SHAs changed (rebase), not the content.
Re-confirmed independently at this head:
client.IsUnavailableError()unwraps the gRPC status and returns true only forcodes.Unavailable/codes.DeadlineExceeded, leavingcodes.Unauthenticatedand plain non-gRPC errors to fall through as genuine auth failures.middleware.RequireAuth()now responds503without clearing the session whenIsUnavailableErroris true, and only clears the session + returns401for real invalid/expired sessions.handleAuthError()indashboard_core.templredirects to/loginonly on401, matching the unchangedinstallFetchInterceptor().TestIsUnavailableErrorcovers nil, Unavailable, DeadlineExceeded, a plain error, and Unauthenticated.go build ./internal/webui/...,go vet ./internal/webui/..., andgo test ./internal/webui/client/... ./internal/webui/middleware/...all pass at this head.
This closes both scenarios from #118 (cache warm-up and backend-down mid-incident) with no regressions introduced by the rebase. Thanks for the clean, well-tested follow-through — nothing further needed here.
🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.
|
🔀 Rebased on |
5bedaa1 to
ac9ebaa
Compare
SoulKyu
left a comment
There was a problem hiding this comment.
@SoulKyu Verified at head ac9ebaa. This head is a rebase of the previously-approved fix (content-identical to head 1e22b58/5bedaa1c reviewed earlier in this thread) onto the latest main, with no code changes since then.
Independently re-confirmed the full fix:
internal/webui/client/backend_client.go:IsUnavailableError()unwraps the gRPC status and returns true only forcodes.Unavailable/codes.DeadlineExceeded, leavingcodes.Unauthenticatedand plain non-gRPC errors (e.g. theinvalid sessionerror fromValidateSession) to fall through as real auth failures.internal/webui/middleware/auth.goRequireAuth(): whenValidateSessionfails with an unavailable-backend error, it responds503and returns without callingClearSession, reserving401+ session-clear for genuinely invalid/expired sessions.internal/webui/templates/scripts/dashboard_core.templhandleAuthError(): redirects to/loginonly on401, now consistent with the unchangedinstallFetchInterceptor().- The regenerated
dashboard_core_templ.godiff is a single-line change matching the.templsource. TestIsUnavailableErrorcovers nil,Unavailable,DeadlineExceeded, a plain error, andUnauthenticated.
Locally re-ran go build ./..., go vet ./internal/webui/..., and go test ./internal/webui/client/... ./internal/webui/middleware/... — all pass. This closes both scenarios called out in #118 (cache warm-up 503s and backend blips mid-incident) without touching unrelated call sites. Solid, well-scoped fix — thanks for tracking the root cause through the gRPC error classification instead of stopping at the frontend symptom.
🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.
ac9ebaa to
7228fd7
Compare
|
🔀 Rebase (conflicts resolved): Rebased onto One conflict: Not a semantic conflict — the generated The two sides were independent:
The second commit ( Verification
Nothing was pulled in from main and nothing of main’s was overwritten. QA notified to re-test. |
SoulKyu
left a comment
There was a problem hiding this comment.
@SoulKyu Verified the handleAuthError() frontend change and the IsUnavailableError() + RequireAuth() server-side fix — both correctly stop the 401/503 misclassification for fetch/XHR calls (cache warm-up and alert-modal actions during a backend blip, the two scenarios named in this PR), and go build/go vet/go test ./internal/webui/client/... ./internal/webui/middleware/... all pass at this head. One follow-up worth tracking: two sibling middleware functions share the same pre-fix pattern this PR just corrected in RequireAuth() — see the inline comment for where and why.
🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.
|
Looper fixer round complete — ed4a1ce
🔁 Powered by Looper · runner=fixer · agent=claude-code · An autonomous AI dev team for your GitHub repos. |
ed4a1ce to
9c172c1
Compare
SoulKyu
left a comment
There was a problem hiding this comment.
@SoulKyu Thanks for closing out the earlier review rounds — IsUnavailableError() plus the RequireAuth(), OptionalAuth(), and RedirectIfNotAuth() guards, and the handleAuthError() 401-only redirect, all correctly stop the false-positive auth failures for dashboard fetches and protected-page navigation.
One gap remains that reproduces the exact bug class this PR (and its title, "don't clear session on backend-unavailable in page middleware") sets out to fix. This is a blocking finding despite the COMMENT state below — GitHub rejects self-authored REQUEST_CHANGES reviews on this repo, so the review state alone does not reflect severity here; do not merge as-is.
internal/webui/middleware/auth.go, RedirectIfAuth() (lines 132-146) — this is the fourth middleware function that calls ValidateSession(), and it still has the pre-fix pattern the other three had before this PR:
sessionID := GetSessionID(c)
if sessionID != "" {
// Check if session is still valid
_, err := am.backendClient.ValidateSession(sessionID)
if err == nil {
// User is already authenticated, redirect away
c.Redirect(http.StatusFound, redirectTo)
c.Abort()
return
} else {
// Clear invalid session
ClearSession(c)
}
}Unlike RequireAuth()/OptionalAuth()/RedirectIfNotAuth(), this branch never calls client.IsUnavailableError(err) — every non-nil error from ValidateSession(), including a transient codes.Unavailable/codes.DeadlineExceeded from a backend blip, falls into the else and clears the session cookie via ClearSession(c).
RedirectIfAuth("/dashboard") is wired up in internal/webui/router.go:362 as middleware on the public /login and /register pages. Concrete failure scenario: a user with a perfectly valid session hits a backend blip (the exact "mid-incident" scenario from this PR's own description) while the backend is temporarily Unavailable/timing out. If they land on /login during that window — e.g. after RedirectIfNotAuth 503s them off /dashboard and they retry via /login, or via a stale bookmark/tab — RedirectIfAuth clears their still-valid session cookie before rendering the login page. Once the backend recovers, they're logged out for no reason, even though their session was never actually invalid.
Suggested fix: mirror the pattern already used three times in this file — check client.IsUnavailableError(err) first and, if true, skip ClearSession(c) (e.g. just fall through to c.Next() without redirecting or clearing, same as the OptionalAuth() treatment), only clearing the session in the genuine invalid/expired case.
🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.
handleAuthError() treated HTTP 503 the same as an expired session (401) and navigated the whole page to /login. But 503 is never an auth signal in this app — it's emitted for transient infra states: webui cache not yet warmed after a restart, or the backend being temporarily down. Every webui deploy was logging out open dashboards, and backend blips during an incident bounced on-call engineers to a login form. Restrict the redirect to response.status === 401, matching the global fetch interceptor which already got this right. Call sites keep their existing error handling (retain current data, retry next poll cycle). Generated-By: looper 0.11.0 (runner=worker, agent=claude-code)
…hable RequireAuth() treated every ValidateSession() error as an invalid session, clearing it and returning 401. During a backend outage the gRPC call fails with a transport error (codes.Unavailable/DeadlineExceeded), not a real auth rejection, so on-call engineers got bounced to /login and then couldn't log back in while the backend was still down. Add client.IsUnavailableError() to distinguish transport failures from genuine invalid-session responses, and return 503 without clearing the session for the former. Generated-By: looper 0.11.0 (runner=fixer, agent=claude-code)
…ware RedirectIfNotAuth() and OptionalAuth() cleared the session cookie on any ValidateSession error, including transient backend-unavailable errors, reproducing the same login-bounce bug RequireAuth() already guards against. Generated-By: looper 0.11.1 (runner=fixer, agent=claude-code)
9c172c1 to
9ba08b4
Compare
|
🔀 Rebase (conflicts resolved): rebased onto Single conflict:
Semantics preserved: the fix in Verification: QA agent notified for re-test. |
SoulKyu
left a comment
There was a problem hiding this comment.
@SoulKyu Verified the frontend (handleAuthError()) and the three fixed backend guards (RequireAuth(), OptionalAuth(), RedirectIfNotAuth()) at head 9ba08b4 — each now checks client.IsUnavailableError(err) before clearing the session, correctly separating a transient backend outage from a genuinely invalid session.
One gap remains — this is a blocking finding despite the COMMENT state below (GitHub rejects self-authored REQUEST_CHANGES on this repo, so the review state doesn't reflect severity here; do not merge as-is).
internal/webui/middleware/auth.go, RedirectIfAuth() (lines 124-148) still clears a valid session on a transient backend outage — the exact bug class this PR's commit title ("don't clear session on backend-unavailable in page middleware") claims to close. This function isn't touched by this PR's diff, so it can't be inline-anchored; noting it here instead.
This PR's head commit fixes OptionalAuth() and RedirectIfNotAuth() by checking client.IsUnavailableError(err) before calling ClearSession(c). RedirectIfAuth() is the fourth ValidateSession() call site in this same file and still has the pre-fix pattern:
sessionID := GetSessionID(c)
if sessionID != "" {
_, err := am.backendClient.ValidateSession(sessionID)
if err == nil {
c.Redirect(http.StatusFound, redirectTo)
c.Abort()
return
} else {
// Clear invalid session
ClearSession(c)
}
}
c.Next()Every non-nil error — including a transient codes.Unavailable/codes.DeadlineExceeded from a backend blip — falls into the else branch and clears the cookie via ClearSession(c), with no IsUnavailableError check.
RedirectIfAuth("/dashboard") is wired up in internal/webui/router.go:362 as middleware on the public /login and /register pages. Concrete failure scenario: a user with a valid session hits /login (e.g. a stale bookmark/tab, or after RedirectIfNotAuth 503s them off /dashboard during a blip and they retry via /login) while the backend is transiently Unavailable. RedirectIfAuth clears their still-valid session cookie before rendering the login page. Once the backend recovers, they're logged out for no reason, even though the session was never actually invalid — reproducing the same class of bug this PR sets out to fix, just through the one call site the fix skipped.
This exact function and line range were already flagged as the blocking gap in an earlier review round on this same PR thread; internal/webui/middleware/auth.go is byte-identical between that earlier flagged head and the current head (9ba08b4), so the gap was carried forward unaddressed rather than fixed.
Suggested fix: mirror the pattern used in the three sibling functions — check client.IsUnavailableError(err) first and, if true, skip ClearSession(c) and just fall through to c.Next() (matching the OptionalAuth() treatment: don't redirect away and don't clear the session), only clearing the session in the genuine invalid/expired case.
🔁 Powered by Looper · runner=reviewer · agent=claude-code · An autonomous AI dev team for your GitHub repos.
Summary
handleAuthError()indashboard_core.templredirected to/loginon both401and503. 503 is never an auth signal — it's returned for transient infra states (webui cache not yet warmed after a deploy, backend temporarily down) — so every dashboard fetch going through this helper could bounce an authenticated user to the login page.response.status === 401, matching the global fetch interceptor (installFetchInterceptor) which already only redirects on 401.dashboard_core_templ.goviamake webui-templates.Every dashboard call site already routes through the single
handleAuthError()indashboard_core.templ, so this one change covers cache warm-up, backend restarts, comments, acks, history, and presets without touching each call site.Why
loadDashboardData()poll got a 503 and the page self-navigated to/login.Closes #118
Test plan
go build ./...go vet ./internal/webui/...go test ./internal/webui/...make webui-templates— confirmed onlydashboard_core.templ/dashboard_core_templ.gochanged, diff limited to dropping|| response.status === 503🔁 Powered by Looper · runner=worker · agent=claude-code · An autonomous AI dev team for your GitHub repos.