Session expiry: sliding idle timeout with an absolute ceiling (#108) #127

Merged

lz merged 18 commits from feat/session-expiry into main 2026-09-05 12:40:32 +02:00

Conversation 0 Commits 18 Files changed 29 +2670 -102

lz commented 2026-09-05 12:11:03 +02:00

Owner

Copy link

Closes #108. Closes #49.

Now targets main — #123 has merged and origin/main is merged in (dde880d). Mergeable, no conflicts.

The symptom

Leave Nexus open a while and it stops accepting operations. Log out, log back in, everything works again. Two independent defects compound.

The TTL was absolute, not sliding. SessionStore.create() stamped expiresAt once and get() never moved it. With session_ttl_hours defaulting to 12, an operator actively using the app was signed out 12 hours after unlocking regardless of activity.

Nothing acted on the resulting 401. The gate returned 401 {"error":"locked"}, call() threw, and no caller distinguished it. The redirect that would rescue you lives in +layout.server.ts, which only runs on a navigation — a polling SPA never triggers one. So the UI just went quiet.

Two details made it look patchy rather than total, which is why it read as mysterious: an already-open terminal keeps streaming after expiry (it authenticates only at handshake), and the quota poll used raw fetch with if (!res.ok) return, so the header kept displaying its last value while the session was dead.

What changed

Two deadlines, one comparison. SessionData gains absoluteExpiresAt (a ceiling, fixed at create()) beside expiresAt (idle, slides). Because expiresAt is clamped to the ceiling at every write, the single expiresAt < now check in get() enforces both — there is no second comparison to add and no way for a later edit to check one and forget the other.

get() stays a pure read; only touch() slides. This is the crux. The refcounted sessionStats store polls /api/quota every 10s while anything is subscribed — the header badge is, on every page the operator can browse — so if any authenticated request slid the deadline, "idle" would degenerate into "no tab open" and the timeout would be decorative. The hook slides only when the method is neither GET nor HEAD — HEAD excluded because SvelteKit serves it from the same GET handler, so admitting it would reopen the hole through any route's HEAD.

POST /api/auth/touch has a deliberately empty handler. It is a non-GET /api/* request, so the hook slides it like any other; a sessions.touch() in the route would be a second, divergeable slide site. It is not in PUBLIC_API — the 401 it earns from a dead session is the client's expiry detector on a page issuing no other authenticated request.

The keepalive has no timer. $lib/keepalive sends on interaction (pointerdown, keydown, focus), throttled to once a minute. An unattended tab sends nothing at all — a periodic ping would measure "tab open" instead of "operator present". It also means the 401 arrives the moment you reach for the app, not minutes later.

The cookie carries the ceiling, not the idle TTL (auth/cookie.ts, now the single place both unlock and rotate-passphrase read). The cookie has to outlive every session it could name: one that expires first sends no sid at all, so a session the server had been happily sliding looks gone to the browser. That drift is the bug.

Every 401 from call() bounces once to /unlock?next=…, skipped on /unlock and /setup so a wrong passphrase doesn't reload the page mid-typo. next is sanitized — //evil.com is protocol-relative, so a browser reads it as absolute while a naive "starts with /" check calls it local, and browsers normalise \ to / in an authority. That closes #49.

Settings: session_ttl_hours is reused as the idle timeout (relabelled in the UI, not renamed in the DB — zero migration); session_max_lifetime_hours is new, default 168.

Three things worth a second look

The merge with #122 moved the fix, and the conflict was semantic. This branch originally routed QuotaBadge's own fetch through call(). On main, #122 had refactored the badge to stop fetching entirely — the poll moved into the refcounted sessionStats store, which took the raw fetch and its if (!res.ok) return with it. So the 401-swallowing bug moved too. Resolution takes main's badge verbatim and puts the fix in SessionStatsStore.refresh() instead. That is strictly better than the original: one poll for the whole app rather than N+1, and still the only authenticated poll that runs wherever the operator can browse — which is what makes it the detector.

The ceiling constructor argument is required, with no default. It started optional, defaulting to Infinity. Review pointed out that's a silent runtime regression waiting to happen: omit the argument and you get a store that slides without bound — strictly worse than the fixed deadline this replaces, with no symptom. A NaN guard was considered and rejected as the wrong defence, since coerce throws on a non-numeric setting rather than yielding NaN; the reachable mistake was always the forgotten argument, and Infinity passes every finiteness check. Required makes it a compile error. Cost: 13 mechanical test edits. Same reasoning as the exhaustive switch in mounts/ (fact #22).

A pre-existing env-vs-setting split is closed. singletons.ts built the store from config.SESSION_TTL_HOURS (env) while the cookie read the instance setting. Nearly harmless while the deadline was absolute; under sliding the in-memory value becomes authoritative, so lowering session_ttl_hours in the UI would have done nothing. There's now a cachedSessionTtls pair on the singletons object that the store's lazy readers read through, refreshed at boot and on the settings PUT — same cross-bundle pattern as cachedHosts, for the same reason.

Verification

1366 tests across 131 files, typecheck (4922 files) and lint clean — post-merge, on the tree as it stands.

Driven in a real browser against a built server, since .svelte files have no test coverage by construction (vitest.config.ts is environment: 'node'):

Check	Result
Restart Nexus while sitting on a page	the quota poll alone bounced to /unlock?next=%2Fsettings%3Ftab%3Dinstance — no click needed
Unlock afterwards	landed back on /settings?tab=instance
Interaction-driven path	keepalive fired on a dead session → bounced, carrying next=%2Fclaude-session
?next=//evil.com	rejected → /
Idle 75s, no interaction	0 keepalive requests
10 interactions in a burst	1 keepalive request
Settings pane	"Session idle timeout (hours)" = 12, "Session max lifetime (hours)" = 168

The restart case is the real #108 scenario — the cookie survives, the in-memory session doesn't.

Negative controls, run and reverted: pointing sessionCookieOptions at the idle setting turns 3 tests red across 2 files; folding the slide into get() turns exactly 1 test red, the one whose comment says so.

Re-verified after the merge. The browser pass above originally ran against the badge's own poll, which no longer exists. Re-driven on the post-merge build: the sessionStats poll confirmed live on /settings (2 requests in 25s), then a server restart with no interaction at all bounced to /unlock?next=%2Fsettings%3Ftab%3Dinstance, and unlocking returned to /settings?tab=instance.

Notes for review

• Sessions still don't survive a restart, by design. SessionData.key is the Argon2id-derived vault key; persisting a session without it yields one that is authenticated-but-locked — the same failure with more code. Where key material could live across a restart is #110 item 2. The 401 handling is what makes a restart graceful rather than mysterious.
• The terminal WS handshake calls get() and therefore does not slide. Deliberate: SessionTerminal.svelte reconnects on a backoff timer, and a retry loop is not a present human.
• No cross-field validator enforcing ceiling ≥ idle. A ceiling below the idle timeout yields a session with a short fixed lifetime, which is what the operator literally configured; machinery to prevent it would cost more than the confusion it saves.
• The terminal overlay's misleading "the workspace may not be running" string is untouched — it genuinely can't distinguish 401 from 409, since a browser exposes a rejected WS handshake as bare close code 1006. With this change the app navigates to /unlock before an operator reads it.
• session_max_lifetime_hours deliberately has no env var: a new setting would inherit the trap (an env value silently freezes the field in the UI) without the backwards-compatibility that justifies it on session_ttl_hours.
• The plan document is included; AGENTS.md fact 25 records the model. Three docs: commits amend the plan mid-flight — each records a defect review found in the plan text itself, so the committed plan matches what was built rather than what was first specified.

https://claude.ai/code/session_01VY9PGWAqaA9Gf7fLeXPLjW

Closes #108. Closes #49. Now targets `main` — #123 has merged and `origin/main` is merged in (`dde880d`). Mergeable, no conflicts. ## The symptom Leave Nexus open a while and it stops accepting operations. Log out, log back in, everything works again. Two independent defects compound. **The TTL was absolute, not sliding.** `SessionStore.create()` stamped `expiresAt` once and `get()` never moved it. With `session_ttl_hours` defaulting to 12, an operator actively *using* the app was signed out 12 hours after unlocking regardless of activity. **Nothing acted on the resulting 401.** The gate returned `401 {"error":"locked"}`, `call()` threw, and no caller distinguished it. The redirect that would rescue you lives in `+layout.server.ts`, which only runs on a navigation — a polling SPA never triggers one. So the UI just went quiet. Two details made it look patchy rather than total, which is why it read as mysterious: an already-open terminal keeps streaming after expiry (it authenticates only at handshake), and the quota poll used raw `fetch` with `if (!res.ok) return`, so the header kept displaying its last value while the session was dead. ## What changed **Two deadlines, one comparison.** `SessionData` gains `absoluteExpiresAt` (a ceiling, fixed at `create()`) beside `expiresAt` (idle, slides). Because `expiresAt` is clamped to the ceiling at every write, the single `expiresAt < now` check in `get()` enforces *both* — there is no second comparison to add and no way for a later edit to check one and forget the other. **`get()` stays a pure read; only `touch()` slides.** This is the crux. The refcounted `sessionStats` store polls `/api/quota` every 10s while anything is subscribed — the header badge is, on every page the operator can browse — so if any authenticated request slid the deadline, "idle" would degenerate into "no tab open" and the timeout would be decorative. The hook slides only when the method is neither GET nor HEAD — HEAD excluded because SvelteKit serves it from the same GET handler, so admitting it would reopen the hole through any route's HEAD. **`POST /api/auth/touch` has a deliberately empty handler.** It is a non-GET `/api/*` request, so the hook slides it like any other; a `sessions.touch()` in the route would be a second, divergeable slide site. It is *not* in `PUBLIC_API` — the 401 it earns from a dead session is the client's expiry detector on a page issuing no other authenticated request. **The keepalive has no timer.** `$lib/keepalive` sends on interaction (`pointerdown`, `keydown`, `focus`), throttled to once a minute. An unattended tab sends nothing at all — a periodic ping would measure "tab open" instead of "operator present". It also means the 401 arrives the moment you reach for the app, not minutes later. **The cookie carries the ceiling, not the idle TTL** (`auth/cookie.ts`, now the single place both `unlock` and `rotate-passphrase` read). The cookie has to outlive every session it could name: one that expires first sends no `sid` at all, so a session the server had been happily sliding looks *gone* to the browser. That drift is the bug. **Every 401 from `call()` bounces once** to `/unlock?next=…`, skipped on `/unlock` and `/setup` so a wrong passphrase doesn't reload the page mid-typo. `next` is sanitized — `//evil.com` is protocol-relative, so a browser reads it as absolute while a naive "starts with `/`" check calls it local, and browsers normalise `\` to `/` in an authority. That closes #49. **Settings:** `session_ttl_hours` is reused as the idle timeout (relabelled in the UI, not renamed in the DB — zero migration); `session_max_lifetime_hours` is new, default 168. ## Three things worth a second look **The merge with #122 moved the fix, and the conflict was semantic.** This branch originally routed `QuotaBadge`'s own `fetch` through `call()`. On `main`, #122 had refactored the badge to stop fetching entirely — the poll moved into the refcounted `sessionStats` store, which took the raw `fetch` and its `if (!res.ok) return` with it. So the 401-swallowing bug moved too. Resolution takes main's badge verbatim and puts the fix in `SessionStatsStore.refresh()` instead. That is strictly better than the original: one poll for the whole app rather than N+1, and still the only authenticated poll that runs wherever the operator can browse — which is what makes it the detector. **The ceiling constructor argument is required, with no default.** It started optional, defaulting to `Infinity`. Review pointed out that's a silent runtime regression waiting to happen: omit the argument and you get a store that slides *without bound* — strictly worse than the fixed deadline this replaces, with no symptom. A `NaN` guard was considered and rejected as the wrong defence, since `coerce` throws on a non-numeric setting rather than yielding `NaN`; the reachable mistake was always the forgotten argument, and `Infinity` passes every finiteness check. Required makes it a compile error. Cost: 13 mechanical test edits. Same reasoning as the exhaustive `switch` in `mounts/` (fact #22). **A pre-existing env-vs-setting split is closed.** `singletons.ts` built the store from `config.SESSION_TTL_HOURS` (env) while the cookie read the instance setting. Nearly harmless while the deadline was absolute; under sliding the in-memory value becomes authoritative, so lowering `session_ttl_hours` in the UI would have done *nothing*. There's now a `cachedSessionTtls` pair on the singletons object that the store's lazy readers read *through*, refreshed at boot and on the settings PUT — same cross-bundle pattern as `cachedHosts`, for the same reason. ## Verification **1366 tests** across 131 files, typecheck (4922 files) and lint clean — post-merge, on the tree as it stands. Driven in a real browser against a built server, since `.svelte` files have no test coverage by construction (`vitest.config.ts` is `environment: 'node'`): | Check | Result | |---|---| | Restart Nexus while sitting on a page | the quota poll **alone** bounced to `/unlock?next=%2Fsettings%3Ftab%3Dinstance` — no click needed | | Unlock afterwards | landed back on `/settings?tab=instance` | | Interaction-driven path | keepalive fired on a dead session → bounced, carrying `next=%2Fclaude-session` | | `?next=//evil.com` | rejected → `/` | | Idle 75s, no interaction | **0** keepalive requests | | 10 interactions in a burst | **1** keepalive request | | Settings pane | "Session idle timeout (hours)" = 12, "Session max lifetime (hours)" = 168 | The restart case is the real #108 scenario — the cookie survives, the in-memory session doesn't. Negative controls, run and reverted: pointing `sessionCookieOptions` at the idle setting turns 3 tests red across 2 files; folding the slide into `get()` turns exactly 1 test red, the one whose comment says so. **Re-verified after the merge.** The browser pass above originally ran against the badge's own poll, which no longer exists. Re-driven on the post-merge build: the `sessionStats` poll confirmed live on `/settings` (2 requests in 25s), then a server restart with **no interaction at all** bounced to `/unlock?next=%2Fsettings%3Ftab%3Dinstance`, and unlocking returned to `/settings?tab=instance`. ## Notes for review - **Sessions still don't survive a restart, by design.** `SessionData.key` *is* the Argon2id-derived vault key; persisting a session without it yields one that is authenticated-but-locked — the same failure with more code. Where key material could live across a restart is #110 item 2. The 401 handling is what makes a restart graceful rather than mysterious. - The terminal WS handshake calls `get()` and therefore does **not** slide. Deliberate: `SessionTerminal.svelte` reconnects on a backoff timer, and a retry loop is not a present human. - No cross-field validator enforcing ceiling ≥ idle. A ceiling below the idle timeout yields a session with a short fixed lifetime, which is what the operator literally configured; machinery to prevent it would cost more than the confusion it saves. - The terminal overlay's misleading "the workspace may not be running" string is untouched — it genuinely can't distinguish 401 from 409, since a browser exposes a rejected WS handshake as bare close code 1006. With this change the app navigates to `/unlock` before an operator reads it. - `session_max_lifetime_hours` deliberately has no env var: a new setting would inherit the trap (an env value silently freezes the field in the UI) without the backwards-compatibility that justifies it on `session_ttl_hours`. - The plan document is included; `AGENTS.md` fact 25 records the model. Three `docs:` commits amend the plan mid-flight — each records a defect review found in the plan text itself, so the committed plan matches what was built rather than what was first specified. https://claude.ai/code/session_01VY9PGWAqaA9Gf7fLeXPLjW

lz added 17 commits 2026-09-05 12:11:06 +02:00

docs: implementation plan for session expiry (#108) 3461da4d0c

feat(auth): give sessions an absolute ceiling and an explicit touch() 704ef2ca97

fix(auth): correct three over-claiming comments and seal get() 277add292f

docs: make the session ceiling a required constructor argument in the plan 3060c19806

docs: flag the constructor comment that goes stale in task 3 dbc9265305

docs: complete the task 3 stale-comment list and enumerate the call sites 298ef1a42b

feat(settings): add session_max_lifetime_hours 75dd826060

refactor(settings): share the session hours validator, pin the def f07b5c3144

feat(auth): read session durations from instance settings, not env 5a00746fdd

docs: wrap the duration refresh in task 3, with the reason 7a32fb0b84

fix(settings): report a failed duration refresh instead of a bare 500 9c10a70cdd

feat(auth): cookie maxAge is the absolute ceiling, decided in one place 914627c25b

feat(auth): slide the session on a cookie-authenticated mutation 3cb4f0f379

feat(auth): activity-driven session keepalive e6133ca848

feat(auth): bounce a 401 to /unlock and come back d5aeab93a8

feat(auth): keepalive, quota through call(), and return to next after unlock dc0c4a4da6

docs: record the session expiry model as AGENTS.md fact 25 75dc40d5d7

lz changed target branch from fix/session to main 2026-09-05 12:13:47 +02:00

lz added 1 commit 2026-09-05 12:29:19 +02:00

Merge remote-tracking branch 'origin/main' into feat/session-expiry ... dde880d265

# Conflicts:
#	nexus/src/lib/components/QuotaBadge.svelte

lz merged commit b656dbe0e4 into main 2026-09-05 12:40:32 +02:00

lz referenced this pull request from a commit 2026-09-05 12:40:35 +02:00

Merge pull request 'Session expiry: sliding idle timeout with an absolute ceiling (#108)' (#127) from feat/session-expiry into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

bug

Something is not working

  

duplicate

This issue or pull request already exists

  

enhancement

New feature

  

help wanted

Need some help

  

invalid

Something is wrong

  

question

More information is needed

  

wontfix

This won't be fixed

No labels

bug

duplicate

enhancement

help wanted

invalid

question

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

lz/agent-nexus!127

No description provided.