fix(observability): clear the dead session on the first bounce, stop filing client disconnects as exceptions, scope the Hem probe (#2114)

Three small defects found underneath error clusters that are themselves benign.

bounceToAuth returned a bare NextResponse.redirect and never copied the cookies
off supabaseResponse, so the Set-Cookie headers that clear a dead session were
thrown away on the first bounce and the browser replayed the dead refresh token
once more on /login. That doubled both the GoTrue 400s and the log volume. The
AuthApiError itself is left alone: it is correct session-expiry handling that
auth-js logs from inside node_modules, and getUser() returns it as a value.

"The destination stream closed early." is a client disconnecting mid-stream,
produced inside React's Flight server. Next's own isAbortError filter does not
recognise React's cancel error, so instrumentation.ts reported it to PostHog
Error Tracking as a real exception against real users' session replays and paid
an awaited flush on an otherwise-healthy request. A narrow predicate now
early-returns before PostHog is touched. This cannot remove the line from
Vercel's runtime-error table, which is fed by Next's stderr.

other-account-hint.ts issued an unfiltered journal_entries probe on the
blocking Hem render path, inside a render Promise.all: roughly 1 in 20 Hem
loads waited an extra 2.5 s for an advisory nudge. An unfiltered probe on a
multi-tenant table is also a correctness smell. It is now company-scoped and
off the blocking path.


Claude-Session: https://claude.ai/code/session_016ifKg6Ec67A39oxfGPU1yc

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Jakob Wennberg
2026-09-01 15:03:23 +02:00
committed by GitHub
parent f1d76deaba
commit b1f7116231
7 changed files with 353 additions and 46 deletions
+13
View File
@@ -1,4 +1,8 @@
import type { Instrumentation } from 'next'
// Imported by exact path rather than through the `@/lib/observability` barrel:
// this file's module graph is loaded in every runtime Next boots, and the
// predicate is a dependency-free pure function.
import { isClientDisconnectError } from '@/lib/observability/is-client-disconnect'
export async function register() {
// Instrumentation hook: currently a no-op.
@@ -23,8 +27,17 @@ export async function register() {
* instrumentation-client.ts), which is what links a server error back to the
* user's session replay. Absent that header the error is still captured, just
* unattributed.
*
* A client that navigated away mid-stream is filtered out first: see
* `isClientDisconnectError`. Next reports those through this hook even though
* the response itself succeeded, and they are not exceptions. Next still
* writes its own stderr line for them, so they stay visible in Vercel's
* runtime-error table; what this drops is the false entry in Error Tracking
* and the awaited flush it would cost a healthy request.
*/
export const onRequestError: Instrumentation.onRequestError = async (err, request) => {
if (isClientDisconnectError(err)) return
try {
const { getPostHogServer, flushAnalytics } = await import('@/lib/analytics/posthog-server')
const posthog = getPostHogServer()
@@ -10,20 +10,39 @@ import type { SupabaseClient } from '@supabase/supabase-js'
const mockCreateServiceClient = vi.mocked(createServiceClient)
interface QueryResult {
data?: unknown
error?: unknown
}
/**
* Chainable query mock keyed by table name: every method returns the chain,
* awaiting it resolves with the configured { data, error } for that table.
* Chainable query mock keyed by table name: every method returns the chain and
* records its arguments, and awaiting it resolves with the configured
* { data, error } for that table. A table may be given an ARRAY of results,
* consumed one per from() call with the last one repeating, so the chunked
* existence probe can be answered chunk by chunk.
*/
function buildClient(resultsByTable: Record<string, { data?: unknown; error?: unknown }>) {
function buildClient(resultsByTable: Record<string, QueryResult | QueryResult[]>) {
const queues = new Map<string, QueryResult[]>(
Object.entries(resultsByTable).map(([table, value]) => [
table,
Array.isArray(value) ? [...value] : [value],
]),
)
const calls: Array<{ table: string; method: string; args: unknown[] }> = []
return {
calls,
from: vi.fn((table: string) => {
const result = {
data: resultsByTable[table]?.data ?? null,
error: resultsByTable[table]?.error ?? null,
}
const queue = queues.get(table) ?? []
const configured = (queue.length > 1 ? queue.shift() : queue[0]) ?? {}
const result = { data: configured.data ?? null, error: configured.error ?? null }
const chain: Record<string, unknown> = {}
for (const m of ['select', 'eq', 'in', 'is', 'limit', 'order', 'range']) {
chain[m] = () => chain
chain[m] = (...args: unknown[]) => {
calls.push({ table, method: m, args })
return chain
}
}
;(chain as { then?: unknown }).then = (resolve: (v: unknown) => void) => resolve(result)
return chain
@@ -31,6 +50,11 @@ function buildClient(resultsByTable: Record<string, { data?: unknown; error?: un
}
}
/** The company filters the journal_entries existence probes were issued with. */
function probeFilters(client: ReturnType<typeof buildClient>) {
return client.calls.filter((c) => c.table === 'journal_entries' && c.method === 'in')
}
const OWN_COMPANY = { id: 'own-co', org_number: '5560125790' }
beforeEach(() => {
@@ -50,6 +74,36 @@ describe('shouldShowOtherAccountHint', () => {
expect(mockCreateServiceClient).not.toHaveBeenCalled()
})
it('scopes the own-account probe to the caller\'s companies', async () => {
// Without company_id the probe falls back on the RLS qual, which under the
// authenticated role turns into a sequential scan of journal_entries on
// the blocking Hem render.
const supabase = buildClient({
companies: { data: [OWN_COMPANY] },
journal_entries: { data: [{ id: 'je1' }] },
})
await expect(shouldShowOtherAccountHint(supabase as unknown as SupabaseClient)).resolves.toBe(false)
expect(probeFilters(supabase).map((c) => c.args)).toEqual([['company_id', ['own-co']]])
})
it('chunks the probe and stops at the first company with entries', async () => {
const many = Array.from({ length: 250 }, (_, i) => ({
id: `co-${i}`,
org_number: '5560125790',
}))
const supabase = buildClient({
companies: { data: many },
// First chunk finds nothing, second one does: the third must never run.
journal_entries: [{ data: [] }, { data: [{ id: 'je1' }] }, { data: [] }],
})
await expect(shouldShowOtherAccountHint(supabase as unknown as SupabaseClient)).resolves.toBe(false)
const chunks = probeFilters(supabase).map((c) => (c.args[1] as string[]).length)
expect(chunks).toEqual([100, 100])
expect(mockCreateServiceClient).not.toHaveBeenCalled()
})
it('is true when the account is empty and a same-orgnr company elsewhere has entries', async () => {
const supabase = buildClient({
companies: { data: [OWN_COMPANY] },
+55 -23
View File
@@ -2,6 +2,38 @@ import type { SupabaseClient } from '@supabase/supabase-js'
import { createServiceClient } from '@/lib/supabase/server'
import { fetchAllRows } from '@/lib/supabase/fetch-all'
/**
* Ids per existence probe. They travel in the PostgREST query string as
* `company_id=in.(uuid,uuid,...)`, so an unbounded list would eventually run
* past the gateway's request-header ceiling and be rejected outright. 100
* UUIDs is roughly 3.7 KB, comfortably inside it.
*/
const PROBE_CHUNK_SIZE = 100
/**
* Does any of these companies have at least one journal entry?
*
* The company filter is what makes this an index probe: naming the ids lets
* Postgres use idx_journal_entries_company_id. Stops at the first chunk that
* finds a row. Throws on a query error, which the caller turns into "no hint".
*/
async function hasAnyJournalEntry(
client: SupabaseClient,
companyIds: string[],
): Promise<boolean> {
for (let i = 0; i < companyIds.length; i += PROBE_CHUNK_SIZE) {
const { data, error } = await client
.from('journal_entries')
.select('id')
.in('company_id', companyIds.slice(i, i + PROBE_CHUNK_SIZE))
.limit(1)
if (error) throw new Error(error.message)
if ((data ?? []).length > 0) return true
}
return false
}
/**
* Should the Hem page hint that the user may be signed in to the wrong
* account? (#1231, the "Chillen" support case: BankID resolved to a stale
@@ -15,35 +47,41 @@ import { fetchAllRows } from '@/lib/supabase/fetch-all'
* NOT a member of, has at least one journal entry.
*
* The common case (an account with any bookkeeping at all) exits after one
* indexed existence probe. The cross-account probe runs on the service
* company-scoped existence probe. The cross-account probe runs on the service
* client but the result reduces to one boolean: nothing about the other
* account is revealed beyond "your bookkeeping may live elsewhere".
* Fails soft to false: this is an advisory line, never worth an error.
*/
export async function shouldShowOtherAccountHint(supabase: SupabaseClient): Promise<boolean> {
try {
// RLS scopes both reads to the caller's memberships. Company lists are
// paginated with fetchAllRows (PostgREST caps at 1000 rows; a byrå user
// can belong to many companies); it throws on error, which the outer
// catch turns into false. The journal_entries read stays a bare
// limit(1): it is an existence probe, not a listing.
const [ownCompanies, { data: ownEntries, error: entriesError }] = await Promise.all([
fetchAllRows<{ id: string; org_number: string | null }>(({ from, to }) =>
// RLS scopes every read here to the caller's memberships. Company lists
// are paginated with fetchAllRows (PostgREST caps at 1000 rows; a byrå
// user can belong to many companies); it throws on error, which the outer
// catch turns into false.
const ownCompanies = await fetchAllRows<{ id: string; org_number: string | null }>(
({ from, to }) =>
supabase
.from('companies')
.select('id, org_number')
.is('archived_at', null)
.order('id')
.range(from, to),
),
supabase.from('journal_entries').select('id').limit(1),
])
)
if (entriesError) return false
if (ownCompanies.length === 0) return false
if ((ownEntries ?? []).length > 0) return false
const ownIds = new Set(ownCompanies.map((c) => c.id))
// The probe needs the company list, so it runs after it rather than beside
// it. Leaving company_id out and letting RLS do the scoping reads as the
// cheaper form and is the opposite: under the `authenticated` role the RLS
// qual becomes a filter on the scan, so Postgres reads all of
// journal_entries (~900 ms measured on prod, 4.2 s worst observed) on the
// blocking Hem render instead of using idx_journal_entries_company_id
// (~2 ms). Naming the ids cannot change the answer: user_company_ids(),
// which journal_entries RLS uses, excludes archived companies, and so does
// the list above.
const ownIds = ownCompanies.map((c) => c.id)
if (await hasAnyJournalEntry(supabase, ownIds)) return false
const orgNumbers = [
...new Set(ownCompanies.map((c) => c.org_number).filter((n): n is string => Boolean(n))),
]
@@ -60,17 +98,11 @@ export async function shouldShowOtherAccountHint(supabase: SupabaseClient): Prom
.range(from, to),
)
const otherIds = sameOrgCompanies.map((c) => c.id).filter((id) => !ownIds.has(id))
const ownIdSet = new Set(ownIds)
const otherIds = sameOrgCompanies.map((c) => c.id).filter((id) => !ownIdSet.has(id))
if (otherIds.length === 0) return false
const { data: otherEntries, error: otherEntriesError } = await service
.from('journal_entries')
.select('id')
.in('company_id', otherIds)
.limit(1)
if (otherEntriesError) return false
return (otherEntries ?? []).length > 0
return await hasAnyJournalEntry(service, otherIds)
} catch {
// Service key unavailable (some self-hosted setups) or transient failure.
return false
@@ -0,0 +1,55 @@
import { describe, it, expect } from 'vitest'
import { isClientDisconnectError } from '../is-client-disconnect'
describe('isClientDisconnectError', () => {
it('matches React\'s cancel error for a stream the client closed', () => {
expect(isClientDisconnectError(new Error('The destination stream closed early.'))).toBe(true)
})
it('matches the sibling cancel error React raises on a write failure', () => {
expect(
isClientDisconnectError(new Error('The destination stream errored while writing data.')),
).toBe(true)
})
it('matches Next\'s own ResponseAborted error by name', () => {
const aborted = Object.assign(new Error('The response was aborted'), {
name: 'ResponseAborted',
})
expect(isClientDisconnectError(aborted)).toBe(true)
})
it('does not match an ordinary error', () => {
expect(isClientDisconnectError(new Error('boom'))).toBe(false)
expect(isClientDisconnectError(new TypeError('fetch failed'))).toBe(false)
})
it('does not match a real provider timeout or socket reset', () => {
// AbortSignal.timeout throws this shape all over lib/providers; treating
// it as a client disconnect would hide genuine integration failures.
expect(isClientDisconnectError(new DOMException('Aborted', 'AbortError'))).toBe(false)
expect(
isClientDisconnectError(Object.assign(new Error('socket hang up'), { code: 'ECONNRESET' })),
).toBe(false)
expect(
isClientDisconnectError(
Object.assign(new Error('Premature close'), { code: 'ERR_STREAM_PREMATURE_CLOSE' }),
),
).toBe(false)
})
it('matches the message exactly, so a real error cannot hide behind the phrase', () => {
expect(
isClientDisconnectError(
new Error('Supabase query failed: The destination stream closed early.'),
),
).toBe(false)
})
it('is false for anything that is not an error object', () => {
expect(isClientDisconnectError(null)).toBe(false)
expect(isClientDisconnectError(undefined)).toBe(false)
expect(isClientDisconnectError('The destination stream closed early.')).toBe(false)
expect(isClientDisconnectError(42)).toBe(false)
})
})
+52
View File
@@ -0,0 +1,52 @@
/**
* Is this error a client hanging up mid-response, rather than a failure?
*
* When a browser navigates away while a page or RSC payload is still
* streaming, the HTTP response closes, Next destroys the stream it handed
* React (`node_modules/next/dist/server/pipe-readable.js`), and React's
* `createCancelHandler` aborts the in-flight render with a bare
* `Error('The destination stream closed early.')`. Both renderers do it: the
* Flight server (react-server-dom-webpack-server.node.production.js:3922) and
* react-dom's SSR streamer (react-dom-server.node.production.js:7822, :8099).
* The response itself still completes: every occurrence we sampled in
* production rode on a 200.
*
* Next means to swallow these. `create-error-handler.js` early-returns on
* `isAbortError`, but that predicate knows only `name === 'AbortError'` and
* `name === 'ResponseAborted'` (pipe-readable.js:32), and React's cancel error
* is a plain `Error`, so it falls through to `onRequestError` and is reported
* as an exception that never happened. This predicate closes that gap on our
* side. It is a Next filtering gap, not a version bug: 16.3.1 and 16.3.4 are
* byte-identical here, so upgrading is not the fix.
*
* Matching is on the EXACT message. A React rewording brings the noise back,
* which is the safe direction; a substring match would let a real error hide
* behind the phrase.
*
* Deliberately NOT matched: a bare `AbortError`, `ECONNRESET`,
* `ERR_STREAM_PREMATURE_CLOSE`. Those are how this codebase's outbound calls
* report genuine failures (`AbortSignal.timeout` in `lib/http/fetch-with-timeout`
* and every provider client; `lib/providers/with-provider-call` treats them as
* retryable provider faults), and swallowing them would hide real integration
* breakage. They also buy nothing here: Next already filters `AbortError`
* before instrumentation ever sees it.
*/
const CLIENT_DISCONNECT_MESSAGES = new Set([
'The destination stream closed early.',
'The destination stream errored while writing data.',
])
/**
* Next's own error for a response the client aborted
* (`server/web/spec-extension/adapters/next-request.js:39`). Unlike a plain
* `AbortError`, this name is only ever produced by that one situation.
*/
const RESPONSE_ABORTED_NAME = 'ResponseAborted'
export function isClientDisconnectError(error: unknown): boolean {
if (typeof error !== 'object' || error === null) return false
const { name, message } = error as { name?: unknown; message?: unknown }
if (name === RESPONSE_ABORTED_NAME) return true
return typeof message === 'string' && CLIENT_DISCONNECT_MESSAGES.has(message)
}
+65 -3
View File
@@ -48,18 +48,44 @@ const state = vi.hoisted(() => ({
// What the mocked isEmailOnBrandAllowlist returns (Rule 2 exemption).
allowlisted: false,
signOut: vi.fn(async () => ({ error: null })),
// Cookies auth-js writes through the `cookies.setAll` callback while
// getUser() runs: the ROTATED tokens after a successful refresh, and the
// maxAge-0 deletions when it removes a dead session. The middleware has to
// carry these onto whatever response it returns.
cookieWrites: [] as Array<{
name: string
value: string
options?: Record<string, unknown>
}>,
// Row returned for user_preferences reads (the auto_logout mint lookup).
userPreferences: null as null | { auto_logout: boolean },
userPreferencesError: null as unknown,
}))
vi.mock('@supabase/ssr', () => ({
createServerClient: vi.fn(() => ({
createServerClient: vi.fn((
_url: string,
_key: string,
options: {
cookies: {
setAll: (
cookies: Array<{
name: string
value: string
options?: Record<string, unknown>
}>,
) => void
}
},
) => ({
auth: {
getUser: vi.fn(async () => ({
getUser: vi.fn(async () => {
if (state.cookieWrites.length > 0) options.cookies.setAll(state.cookieWrites)
return {
data: { user: state.user },
error: state.authError,
})),
}
}),
getClaims: vi.fn(async () => ({
data: { claims: state.sessionId ? { session_id: state.sessionId } : {} },
})),
@@ -176,6 +202,7 @@ describe('updateSession redirect destinations', () => {
state.user = null
state.sessionId = 'session-1'
state.authError = null
state.cookieWrites = []
state.aal = null
state.factors = null
state.company = {
@@ -479,6 +506,30 @@ describe('updateSession redirect destinations', () => {
expect(locationOf(response)).toBe(`${ORIGIN}/login`)
})
it('carries the cookies that clear a dead session', async () => {
// auth-js removes the session inside getUser() and queues the deletion
// on the response. Dropping it on the bounce made the browser replay
// the dead refresh token on /login, spending a second GoTrue 400 per
// expiry (paired 400s ~100 ms apart in production).
state.authError = { name: 'AuthApiError', code: 'refresh_token_not_found' }
state.cookieWrites = [
{ name: 'sb-test-auth-token', value: '', options: { path: '/', maxAge: 0 } },
]
const response = await run('/settings/tax', {
headers: {
cookie: `sb-test-auth-token=dead; ${SESSION_TIMEOUT_COOKIE}=stale`,
},
})
expect(response.status).toBe(307)
expect(new URL(locationOf(response)!).pathname).toBe('/login')
const cleared = response.cookies.get('sb-test-auth-token')
expect(cleared?.value).toBe('')
expect(cleared?.maxAge).toBe(0)
expect(response.cookies.get(SESSION_TIMEOUT_COOKIE)?.value).toBe('')
})
it('drops a request path that normalises to a protocol-relative URL', async () => {
// /..//evil.com normalises to the pathname //evil.com. Reflecting that
// back as ?next= would hand the login page an off-origin destination.
@@ -595,6 +646,17 @@ describe('updateSession redirect destinations', () => {
expect(url.pathname).toBe('/mfa/verify')
expect(url.searchParams.get('returnTo')).toBeNull()
})
it('carries the rotated auth cookie instead of re-minting it next request', async () => {
state.cookieWrites = [
{ name: 'sb-test-auth-token', value: 'rotated', options: { path: '/' } },
]
const response = await run('/settings/tax')
expect(new URL(locationOf(response)!).pathname).toBe('/mfa/verify')
expect(response.cookies.get('sb-test-auth-token')?.value).toBe('rotated')
})
})
describe('forced enrollment bounce to /mfa/enroll', () => {
+49 -10
View File
@@ -267,7 +267,12 @@ async function updateSessionInner(
supabase.auth.mfa.getAuthenticatorAssuranceLevel(),
)
if (aal?.nextLevel === 'aal2' && aal?.currentLevel === 'aal1') {
return NextResponse.json({ error: 'MFA-verifiering krävs.' }, { status: 403 })
const response = NextResponse.json(
{ error: 'MFA-verifiering krävs.' },
{ status: 403 },
)
copyResponseCookies(supabaseResponse, response)
return response
}
}
return supabaseResponse
@@ -352,14 +357,17 @@ async function updateSessionInner(
const destination = carriesDestination
? safeReturnTo(request.nextUrl.searchParams.get('next'), '/')
: '/'
return NextResponse.redirect(new URL(destination, request.url))
return redirectWithAuthCookies(
supabaseResponse,
new URL(destination, request.url),
)
}
return supabaseResponse
}
// Protected routes - require authentication
if (!user) {
return bounceToAuth(request, '/login')
return bounceToAuth(request, supabaseResponse, '/login')
}
// ── Home-domain affinity (WL, founder call 2026-08-05) ──────────────────
@@ -380,7 +388,7 @@ async function updateSessionInner(
request,
)
if (homeOutcome.redirectTo) {
return NextResponse.redirect(homeOutcome.redirectTo)
return redirectWithAuthCookies(supabaseResponse, homeOutcome.redirectTo)
}
if (homeOutcome.cacheOk) {
supabaseResponse.cookies.set(
@@ -408,7 +416,8 @@ async function updateSessionInner(
const mfaTarget = `/mfa/enroll${
innerReturnTo ? `?returnTo=${encodeURIComponent(innerReturnTo)}` : ''
}`
return NextResponse.redirect(
return redirectWithAuthCookies(
supabaseResponse,
new URL(
`/account/set-password?returnTo=${encodeURIComponent(mfaTarget)}`,
request.url,
@@ -452,7 +461,7 @@ async function updateSessionInner(
// User has MFA enrolled but hasn't verified this session → redirect to verify
if (aal?.nextLevel === 'aal2' && aal?.currentLevel === 'aal1') {
return bounceToAuth(request, '/mfa/verify')
return bounceToAuth(request, supabaseResponse, '/mfa/verify')
}
// MFA required but user has no factor enrolled yet → force enrollment
@@ -476,7 +485,7 @@ async function updateSessionInner(
const hasVerifiedFactor = factors?.totp?.some(f => f.status === 'verified')
if (!hasVerifiedFactor) {
return bounceToAuth(request, '/mfa/enroll')
return bounceToAuth(request, supabaseResponse, '/mfa/enroll')
}
}
}
@@ -568,7 +577,7 @@ async function updateSessionInner(
if (isByraNoCompanyAllowed) {
return supabaseResponse
}
return NextResponse.redirect(new URL('/byra', request.url))
return redirectWithAuthCookies(supabaseResponse, new URL('/byra', request.url))
}
// Multi-user seat gate: memberships exist but every one is frozen for
@@ -593,7 +602,10 @@ async function updateSessionInner(
.maybeSingle()
const destination = enrichmentRow ? '/select-company' : '/onboarding'
return NextResponse.redirect(new URL(destination, request.url))
return redirectWithAuthCookies(
supabaseResponse,
new URL(destination, request.url),
)
}
// Set company cookie on the response so downstream requests have it
@@ -674,6 +686,28 @@ function copyResponseCookies(from: NextResponse, to: NextResponse): void {
}
}
/**
* Redirect that carries the auth cookies queued on `authResponse`.
*
* auth-js writes through the `setAll` callback while `getUser()` runs: a
* successful refresh puts the ROTATED tokens on the response, a dead session
* puts the cookie DELETIONS there. A bare `NextResponse.redirect()` throws
* both away, so the browser replays the old cookie on the very next request:
* a consumed refresh token on the happy path, and a dead one on the expiry
* path, where production showed the bounce and the following /login each
* spending their own GoTrue 400 about 100 ms apart. Every response that
* replaces `supabaseResponse` has to go through here (or through
* `copyResponseCookies`, for the non-redirect ones).
*/
function redirectWithAuthCookies(
authResponse: NextResponse,
url: URL | string,
): NextResponse {
const response = NextResponse.redirect(url)
copyResponseCookies(authResponse, response)
return response
}
function sessionTimeoutResponse(
request: NextRequest,
authResponse: NextResponse,
@@ -755,9 +789,14 @@ const AUTH_DESTINATION_PARAM = {
* that was going to happen anyway, on exactly the same conditions. The auth
* pages navigate to the destination only after the step-up succeeds, and the
* next request re-runs this same gate regardless.
*
* The bounce also has to carry the cookies auth-js queued on the response
* while `getUser()` ran, hence `authResponse`: see
* `redirectWithAuthCookies`.
*/
function bounceToAuth(
request: NextRequest,
authResponse: NextResponse,
target: keyof typeof AUTH_DESTINATION_PARAM,
) {
// Absolute-path reference: replaces path AND clears query/fragment.
@@ -769,7 +808,7 @@ function bounceToAuth(
if (destination !== '/') {
url.search = `${AUTH_DESTINATION_PARAM[target]}=${encodeURIComponent(destination)}`
}
return NextResponse.redirect(url)
return redirectWithAuthCookies(authResponse, url)
}
/**