Skip to content

fix(api): sign-out fails closed when the session delete does not commit (FIX-LOGOUT) - #131

Merged
vanlongme merged 1 commit into
mainfrom
v10/fix-logout
Oct 6, 2026
Merged

vanlongme merged 1 commit into
mainfrom
v10/fix-logout

Conversation

@vanlongme

Copy link
Copy Markdown
Contributor

Summary

  • Bug: POST /v1/logout logged a session-store Delete error, expired the session cookie and answered 204/200 — while the server-side session stayed live (a copied cookie kept working; /v1/me still 200). The response claimed a sign-out that never committed.
  • Fix: the session-row Delete is now the ordering's point of no return. On a store error the handler answers retryable 503 UNAVAILABLE + Retry-After and tears nothing down — no cookie expiry, no S17 lease/ticket revoke. The refusal's claim ("not signed out — retry") then matches reality: the session still validates and its desktops are still alive.
  • Ordering rationale (S17): delete-first is kept deliberately. Revoking material first would leave a live session with dead streams on delete failure — a half-revoked state a 503 would misreport; expiring the cookie would tell the browser it is signed out while the session still works. On delete success the material revoke runs best-effort exactly as before.
  • Revoke-all (POST /v1/me/sessions:revoke-all) reviewed: already fails closed — the revocation is one transaction; an error answers 500 INTERNAL (retryable: true) and the cookie is only expired after commit. Kept as 500 (changing the pinned test's expectation buys nothing); a new retry test pins the contract.
  • Portal: the sign-out menu already surfaces failures and re-arms the action; en/vi copy now states the user is still signed in and can retry. New unit test proves a 503 refusal shows the toast, stays signed in, and a retry signs out.
  • Contract: POST /v1/logout already listed 503 — the description now documents the fail-closed semantics; schema.d.ts regenerated. Threat-model S17 status line updated.

Test plan

  • TestLogout_StoreDeleteFailureReturns503 (new regression test): fails on v0.5.0 — go test ./internal/api/ -run TestLogout_StoreDeleteFailureReturns503 -count=1 → logout status = 204, want 503 on the pre-fix handler; passes with the fix (503 + Retry-After + UNAVAILABLE/retryable, no Max-Age=0, session still validates, revoker untouched, then a retry completes the destroy and /v1/me → 401).
  • TestRevokeAll_StoreFailureThenRetrySignsOut (new): failure keeps the session; retry commits and the session no longer validates.
  • go test ./internal/api/ -count=1 → ok (all existing logout/S17/revoke-all tests green); go test -race ./internal/api/ -count=1 → ok; go vet ./... clean.
  • npm run test:unit → 305 passed (incl. new "a refused sign-out (503 UNAVAILABLE) can simply be retried"); npm run typecheck, npm run lint:strings clean.

v1.0 fix task, requested by the project orchestrator; the advisor reviews and merges.

Generated with Devin

…it (FIX-LOGOUT)

POST /v1/logout logged a session-store Delete error, expired the cookie
and answered 204/200 while the server-side session stayed live — a copied
cookie kept working. The delete is now the ordering's point of no return:
on failure the handler answers retryable 503 UNAVAILABLE + Retry-After and
tears nothing down (no cookie expiry, no lease/ticket revoke), so the
response never claims a sign-out that did not commit and the still-valid
session keeps its live leases until a retry completes the destroy.

Revoke-all already fails closed (single transaction, 500, cookie kept);
pinned by a retry test that mirrors the real row deletion. Portal copy
(en/vi) now states the user is still signed in and can retry.

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 6, 2026 04:57

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@vanlongme
vanlongme merged commit e199b35 into main Oct 6, 2026
14 checks passed
@vanlongme
vanlongme deleted the v10/fix-logout branch October 6, 2026 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants