Move the logout entry to the top of Completed Steps (part of #256)

The rebase onto next union-merged TODO.md and left this entry below the
one next added.

Model: opus-5-5

README.md

@@ -71,7 +71,8 @@ The state directory (`state_dir`, `/var/lib/pixa` in the container) holds the
 database and the disk cache:
 
 - It needs a persistent volume: without one, every restart starts with an empty
-  cache. In the container, the startup script gives the directory to the user
+  cache. Losing the directory also ends every login session (see Encrypted
+  URLs). In the container, the startup script gives the directory to the user
   pixa runs as (uid 65532) and sets its mode to `750`; outside it, that user
   must be able to write the directory.
 - `cache_max_bytes` limits the source and transformed images together. The
@@ -164,6 +165,14 @@ name.
 Multiple source paths may reference the same content blob; the database tracks
 references rather than using filesystem refcounting.
 
+Source and transformed image files, and the `.meta` file beside each transformed
+image, are written under a temporary name, synced to disk, and only then renamed
+into place, so a crash leaves no file under its own name with part of its data.
+When pixa starts, and every five minutes after, it removes each cached source or
+transformed image whose file is empty, or not the size the database records for
+it, with a warning naming the file; the next request for it fetches the source
+or converts the image again.
+
 pixa's target is 1-5k r/s, which has not been measured at that rate (see Load
 Test). Toward it, pixa keeps in memory the content types of the 10,000
 transformed images most recently cached or served, so a cache hit on one of them
@@ -188,7 +197,11 @@ path under `/v1/` answers 200, in maintenance mode too.
   session it answers 303 to `/`. Answers: 200 with the page showing the URL; 400
   with the page naming a field that is not valid; 500 when the URL cannot be
   made.
-- `GET /logout` — end the login session. Needs: nothing. Answers: 303 to `/`.
+- `POST /logout` — log out with the generator page's Logout button: end every
+  login session, in every browser (see Encrypted URLs), and remove the session
+  cookie. Needs: a login session and the generator page's form (below); without
+  a login session it only removes the cookie. Answers: 303 to `/`; 500 when the
+  logout cannot be saved in the database.
 - `GET` or `HEAD` `/v1/image/<host>/<path>/<size>.<format>`, or
   `/v1/image/<host>/<path>/<size>` with no format — an image, fetched, resized
   and converted (below). Needs: a signature, unless the host is allowlisted (see
@@ -236,7 +249,7 @@ the request carries the same ID as `request_id`, and so do the lines it logs
 when it fetches, converts and serves an image; the fetch sends it to the
 upstream host as `X-Request-ID`.
 
-Both `POST` routes accept only a form that pixa's own page served: the page puts
+The `POST` routes accept only a form that pixa's own page served: the page puts
 a token in the form and sets a cookie to match, and a request without both is
 refused with 403, so another site cannot submit the form from a visitor's
 browser. The login and generator pages are meant to be opened over HTTPS: while
@@ -374,7 +387,15 @@ nor change what it asks for.
 
 1. Open `/` in a browser over HTTPS (or over plain HTTP while `debug` is on, see
    Routes) and log in with the signing key (`signing_key`). The login session
-   lasts 30 days, or until `/logout`.
+   lasts 30 days, or until someone logs out with the generator page's Logout
+   button. Every login is made with the one signing key, so logging out ends
+   every login session, in every browser, a copy of the session cookie included.
+   pixa keeps a random value in its database, which it makes when the database
+   has none and replaces at each logout. A login session carries the value
+   current at its login and is refused once the value has changed, also after a
+   restart (see Deployment). The check for a logout compares no time, so a login
+   after a logout works whatever the host's clock says; the 30 days are still
+   measured by the host's clock.
 2. On the generator page, give the source image's URL, the width and height, the
    format, quality and fit, and how long the URL lasts, then submit the form
    (`POST /generate`). The format is JPEG XL unless another is chosen; a form

TODO.md

@@ -30,6 +30,35 @@ P2: security: per-origin rate limiting
 
 # Completed Steps
 
+- 2026-10-15 logging out ends every login session (closes #256): a login session
+  was a cookie with nothing kept on the server, so `/logout` removed it from one
+  browser and a copy of it kept working for its 30 days. Every login is made
+  with the one signing key, so logging out now ends them all: the session
+  manager keeps a random value, made with `crypto/rand` when the new `logout`
+  table has none and replaced by each logout, saves it there so that it outlasts
+  a restart, puts the value at login in each session, and refuses a session
+  carrying another value. The check for a logout compares no time, so a login
+  after a logout works whatever the host's clock does; a session's 30 days are
+  still checked against the clock. Losing the state directory ends every login
+  session, and no logout can make an ended one work again. Logging out is now
+  `POST /logout` from a form on the generator page, behind the CSRF check the
+  other forms have, so a link or image elsewhere on the same site cannot end the
+  sessions. A logout without a valid login session ends none and only removes
+  the cookie, as anyone can take a CSRF token from the login page. A logout that
+  cannot be saved answers 500. `session.NewManager`, which the tests use, keeps
+  the value in memory only; pixad uses `session.NewManagerWithDatabase`.
+- 2026-10-15 a cached file left empty or partial is no longer served, or fetched
+  again on every request (closes #252). Source and transformed image files, and
+  each transformed image's `.meta` file, are synced to disk before they are
+  renamed into place, and their directory after; the `.meta` file is written
+  before the image, and a failed directory sync removes the renamed file. The
+  reconciliation pass, at startup and every five minutes, removes a cached
+  source or transformed image whose file is empty, or not the size the database
+  records for it, with its rows and a warning naming the file, so the next
+  request fetches the source or converts the image again; requests compare no
+  sizes. Storing a source over a file of another size replaces that file. A
+  source that fails to decode is not removed for that alone: a damaged upstream
+  image would then be fetched on every request.
 - 2026-10-15 a transcode stops once its request's `downstream_timeout` has
   ended, unless it has begun encoding (closes #243): the image processor looked
   at its context only while it waited for a processing slot, so an image was

internal/db/migrations/001_schema.sql

@@ -176,3 +176,11 @@ CREATE TABLE IF NOT EXISTS cache_stats (
     last_updated_at DATETIME DEFAULT CURRENT_TIMESTAMP
 );
 INSERT OR IGNORE INTO cache_stats (id) VALUES (1);
+
+-- A random value that a login session carries from its login. pixa makes it
+-- when the table has no row, and each logout replaces it; a login session
+-- carrying another value is refused, so logging out ends every login session.
+CREATE TABLE IF NOT EXISTS logout (
+    id INTEGER PRIMARY KEY CHECK (id = 1),
+    logout_value TEXT NOT NULL
+);

internal/handlers/auth.go

@@ -89,10 +89,18 @@ func (s *Handlers) handleLoginPost(w http.ResponseWriter, r *http.Request) {
 	http.Redirect(w, r, "/", http.StatusSeeOther)
 }
 
-// HandleLogout clears the session and redirects to login.
+// HandleLogout logs out, which ends every login session (see
+// session.Manager.Logout), and redirects to login.
 func (s *Handlers) HandleLogout() http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
-		s.sessMgr.ClearSession(w)
+		err := s.sessMgr.Logout(w, r)
+		if err != nil {
+			s.log.Error("failed to log out", "error", err)
+			http.Error(w, "Failed to log out", http.StatusInternalServerError)
+
+			return
+		}
+
 		http.Redirect(w, r, "/", http.StatusSeeOther)
 	}
 }

internal/handlers/auth_session_internal_test.go

@@ -185,6 +185,98 @@ func TestHandleLogout_ClearsSessionCookie(t *testing.T) {
 	}
 }
 
+// TestHandleLogout_EndsCopiedSession verifies that after POST /logout a copy
+// of the session cookie taken before it shows the login page, not the
+// generator page.
+func TestHandleLogout_EndsCopiedSession(t *testing.T) {
+	t.Parallel()
+
+	h, srv := newCSRFTestRouter(t)
+
+	copied := newSessionCookie(t, h)
+
+	req := httptest.NewRequestWithContext(
+		t.Context(), http.MethodPost, "/logout", nil)
+	req.AddCookie(copied)
+
+	rec := httptest.NewRecorder()
+	h.HandleLogout().ServeHTTP(rec, req)
+
+	if rec.Code != http.StatusSeeOther {
+		t.Fatalf("POST /logout status = %d, want %d",
+			rec.Code, http.StatusSeeOther)
+	}
+
+	req = httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil)
+	req.AddCookie(copied)
+
+	rec = httptest.NewRecorder()
+	srv.ServeHTTP(rec, req)
+
+	body := rec.Body.String()
+	t.Logf("GET / with the copied cookie: status %d, login page %v",
+		rec.Code, strings.Contains(body, loginForm))
+
+	if !strings.Contains(body, loginForm) || strings.Contains(body, generatorForm) {
+		t.Error("a cookie copied before /logout still opens the generator page")
+	}
+}
+
+// TestHandleLogout_SaveFails_Answers500 verifies that when the logout cannot
+// be saved in the database, POST /logout answers 500, leaves the session
+// cookie in place and ends no session.
+func TestHandleLogout_SaveFails_Answers500(t *testing.T) {
+	t.Parallel()
+
+	h, srv := newCSRFTestRouter(t)
+
+	db := setupTestDB(t)
+
+	sessMgr, err := session.NewManagerWithDatabase(
+		t.Context(), testSigningKey, db)
+	if err != nil {
+		t.Fatalf("session.NewManagerWithDatabase() error = %v", err)
+	}
+
+	h.sessMgr = sessMgr
+	cookie := newSessionCookie(t, h)
+
+	// Saving the logout fails on a closed database.
+	err = db.Close()
+	if err != nil {
+		t.Fatalf("closing the database: %v", err)
+	}
+
+	req := httptest.NewRequestWithContext(
+		t.Context(), http.MethodPost, "/logout", nil)
+	req.AddCookie(cookie)
+
+	rec := httptest.NewRecorder()
+	h.HandleLogout().ServeHTTP(rec, req)
+
+	t.Logf("POST /logout: status %d, Set-Cookie %q",
+		rec.Code, rec.Header().Get("Set-Cookie"))
+
+	if rec.Code != http.StatusInternalServerError {
+		t.Errorf("POST /logout status = %d, want %d",
+			rec.Code, http.StatusInternalServerError)
+	}
+
+	if c := findSessionCookie(rec); c != nil {
+		t.Errorf("a logout that failed set the session cookie: %s", c)
+	}
+
+	req = httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil)
+	req.AddCookie(cookie)
+
+	rec = httptest.NewRecorder()
+	srv.ServeHTTP(rec, req)
+
+	if !strings.Contains(rec.Body.String(), generatorForm) {
+		t.Error("a logout that failed ended the session")
+	}
+}
+
 // TestGeneratePost_NoSession_RedirectsToLogin verifies that POST /generate
 // with a valid CSRF token but no login session answers 303 to / and makes no
 // URL.

internal/handlers/handlers.go

@@ -164,7 +164,10 @@ func (s *Handlers) initImageService() error {
 
 	// Initialize session manager (signing key is validated at config load
 	// time). Session cookies are always Secure/HttpOnly/SameSite=Strict.
-	sessMgr, err := session.NewManager(s.config.SigningKey)
+	// The random value that each logout replaces is saved in the database,
+	// so it outlasts a restart.
+	sessMgr, err := session.NewManagerWithDatabase(
+		context.Background(), s.config.SigningKey, s.db.DB())
 	if err != nil {
 		return err
 	}

internal/imgcache/cache.go

@@ -660,10 +660,10 @@ func (c *Cache) readHitAndMissCounts(ctx context.Context, stats *CacheStats) err
 // hold: it reads the content type from the variant's .meta file and
 // keeps it in metaCache, unless a StoreVariant has put one there
 // meanwhile, as the store's is newer. A read that finds no .meta file,
-// as one can between a store's writing of the variant file and of its
+// as one can when the store's .meta write failed or an eviction removed
-// .meta file, serves application/octet-stream and keeps nothing, so
+// the file after the read opened the variant, serves
-// metaCache only ever holds a type read from a .meta file or passed to
+// application/octet-stream and keeps nothing, so metaCache only ever
-// StoreVariant.
+// holds a type read from a .meta file or passed to StoreVariant.
 func (c *Cache) loadVariantWithMeta(
 	cacheKey VariantKey,
 ) (io.ReadCloser, int64, string, error) {

internal/imgcache/eviction.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"database/sql"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io/fs"
 	"os"
@@ -560,10 +561,11 @@ func (c *Cache) runCleanExpired(ctx context.Context) {
 // (or whose accounting insert failed, e.g. StoreVariant's best-effort
 // insert under transient DB contention), drops accounting rows whose
 // files are missing, removes source blob files the database does not
-// know (and rows whose files are gone), sweeps stale temp files left
+// know (and rows whose files are gone), removes source and variant files
-// behind by crashed writes, and last checks the total cache usage
+// that are empty or not the size their rows record, sweeps stale temp
-// against the tables. It reads the tables a page at a time. Running it
+// files left behind by crashed writes, and last checks the total cache
-// periodically, not just once, bounds how long such drift can
+// usage against the tables. It reads the tables a page at a time.
+// Running it periodically, not just once, bounds how long such drift can
 // accumulate unaccounted for on a long-running process to one eviction
 // interval. Once ctx is cancelled, it stops at the next file or row and
 // returns ctx's error.
@@ -595,8 +597,9 @@ func (c *Cache) reconcileAccounting(ctx context.Context) error {
 	return c.reconcileUsageTotal(ctx)
 }
 
-// reconcileVariantFiles walks the variant storage directory, adopting
+// reconcileVariantFiles walks the variant storage directory, checking
-// files without accounting rows and sweeping stale temp files.
+// each variant file against its accounting row (reconcileVariantFile)
+// and sweeping stale temp files.
 func (c *Cache) reconcileVariantFiles(ctx context.Context) error {
 	return filepath.WalkDir(
 		c.variants.baseDir,
@@ -621,38 +624,55 @@ func (c *Cache) reconcileVariantFiles(ctx context.Context) error {
 				return nil
 			}
 
-			return c.adoptVariantFile(ctx, path, entry, VariantKey(name))
+			return c.reconcileVariantFile(ctx, path, entry, VariantKey(name))
 		},
 	)
 }
 
-// adoptVariantFile inserts an accounting row for a variant file that
+// reconcileVariantFile checks one variant file against its accounting
-// has none, using the file's size and modification time.
+// row. A file without a row is adopted into the accounting, unless it is
-func (c *Cache) adoptVariantFile(
+// empty. A file that is empty, or not the size its row records, as a
+// crash or a restored snapshot can leave it, is removed with its row and
+// a warning naming it, so the variant is converted again.
+func (c *Cache) reconcileVariantFile(
 	ctx context.Context, path string, entry fs.DirEntry, cacheKey VariantKey,
 ) error {
-	var rowExists int
-
-	err := c.db.QueryRowContext(ctx,
-		`SELECT COUNT(*) FROM variant_content WHERE cache_key = ?`, string(cacheKey),
-	).Scan(&rowExists)
-	if err != nil {
-		return fmt.Errorf("failed to check variant accounting row: %w", err)
-	}
-
-	if rowExists > 0 {
-		return nil
-	}
-
 	info, err := entry.Info()
 	if err != nil {
 		return fmt.Errorf("failed to stat variant file: %w", err)
 	}
 
+	var recordedSize int64
+
+	err = c.db.QueryRowContext(ctx,
+		`SELECT size_bytes FROM variant_content WHERE cache_key = ?`, string(cacheKey),
+	).Scan(&recordedSize)
+
+	switch {
+	case errors.Is(err, sql.ErrNoRows) && info.Size() > 0:
+		return c.adoptVariantFile(ctx, path, info, cacheKey)
+	case err != nil && !errors.Is(err, sql.ErrNoRows):
+		return fmt.Errorf("failed to check variant accounting row: %w", err)
+	case info.Size() > 0 && info.Size() == recordedSize:
+		return nil
+	}
+
+	// The file is empty, or not the size its row records
+	c.log.Warn("removing damaged cached variant file", "path", path,
+		"file_bytes", info.Size(), "recorded_bytes", recordedSize)
+
+	return c.evictVariant(ctx, cacheKey)
+}
+
+// adoptVariantFile inserts an accounting row for a variant file that
+// has none, using the file's size and modification time.
+func (c *Cache) adoptVariantFile(
+	ctx context.Context, path string, info fs.FileInfo, cacheKey VariantKey,
+) error {
 	modTime := info.ModTime().UTC().Format(sqliteTimestampLayout)
 	contentType := c.variantContentTypeFromSidecar(path)
 
-	_, err = c.db.ExecContext(ctx, `
+	_, err := c.db.ExecContext(ctx, `
 		INSERT INTO variant_content
 			(cache_key, size_bytes, content_type, created_at, last_accessed_at)
 		VALUES (?, ?, ?, ?, ?)
@@ -771,10 +791,9 @@ func queryStringColumn[T ~string](
 	return values, nil
 }
 
-// reconcileSourceFiles walks the source content directory, removing
+// reconcileSourceFiles walks the source content directory, checking
-// blob files the database does not track (they are unreachable: source
+// each blob file against its source_content row (reconcileSourceFile)
-// lookups always go through source_metadata) and sweeping stale temp
+// and sweeping stale temp files.
-// files.
 func (c *Cache) reconcileSourceFiles(ctx context.Context) error {
 	return filepath.WalkDir(
 		c.srcContent.baseDir,
@@ -795,31 +814,55 @@ func (c *Cache) reconcileSourceFiles(ctx context.Context) error {
 				return nil
 			}
 
-			return c.removeUntrackedSourceFile(ctx, path, ContentHash(name))
+			return c.reconcileSourceFile(ctx, path, entry, ContentHash(name))
 		},
 	)
 }
 
+// reconcileSourceFile checks one source blob file against its
+// source_content row. A file without a row is removed: it is
+// unreachable, as source lookups always go through source_metadata. A
+// file that is empty, or not the size its row records, as a crash or a
+// restored snapshot can leave it, is removed with its rows and a warning
+// naming it, so the source is fetched again.
+func (c *Cache) reconcileSourceFile(
+	ctx context.Context, path string, entry fs.DirEntry, contentHash ContentHash,
+) error {
+	info, err := entry.Info()
+	if err != nil {
+		return fmt.Errorf("failed to stat source content file: %w", err)
+	}
+
+	var recordedSize int64
+
+	err = c.db.QueryRowContext(ctx,
+		`SELECT size_bytes FROM source_content WHERE content_hash = ?`,
+		string(contentHash),
+	).Scan(&recordedSize)
+
+	switch {
+	case errors.Is(err, sql.ErrNoRows):
+		return c.removeUntrackedSourceFile(ctx, path, contentHash)
+	case err != nil:
+		return fmt.Errorf("failed to check source content row: %w", err)
+	case info.Size() > 0 && info.Size() == recordedSize:
+		return nil
+	}
+
+	// The file is empty, or not the size its row records
+	c.log.Warn("removing damaged cached source file", "path", path,
+		"file_bytes", info.Size(), "recorded_bytes", recordedSize)
+
+	return c.evictSourceBlob(ctx, contentHash)
+}
+
 // removeUntrackedSourceFile deletes a source blob file that has no
 // source_content row. Any source_metadata rows referencing the hash
 // are removed first so no row ever points at a deleted file.
 func (c *Cache) removeUntrackedSourceFile(
 	ctx context.Context, path string, contentHash ContentHash,
 ) error {
-	var rowExists int
+	_, err := c.db.ExecContext(ctx,
-
-	err := c.db.QueryRowContext(ctx,
-		`SELECT COUNT(*) FROM source_content WHERE content_hash = ?`, string(contentHash),
-	).Scan(&rowExists)
-	if err != nil {
-		return fmt.Errorf("failed to check source content row: %w", err)
-	}
-
-	if rowExists > 0 {
-		return nil
-	}
-
-	_, err = c.db.ExecContext(ctx,
 		`DELETE FROM source_metadata WHERE content_hash = ?`, string(contentHash))
 	if err != nil {
 		return fmt.Errorf("failed to delete metadata rows for untracked blob: %w", err)

internal/imgcache/partial_cache_files_internal_test.go

@@ -0,0 +1,359 @@
+package imgcache
+
+import (
+	"bytes"
+	"crypto/sha256"
+	"encoding/hex"
+	"io"
+	"log/slog"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// getImage calls Get for req and returns the image it answered with and
+// its cache status.
+func getImage(t *testing.T, svc *Service, req *ImageRequest) ([]byte, CacheStatus) {
+	t.Helper()
+
+	resp, err := svc.Get(t.Context(), req)
+	if err != nil {
+		t.Fatalf("Get() of quality %d error = %v", req.Quality, err)
+	}
+
+	defer func() { _ = resp.Content.Close() }()
+
+	body, err := io.ReadAll(resp.Content)
+	if err != nil {
+		t.Fatalf("reading the image of quality %d: %v", req.Quality, err)
+	}
+
+	return body, resp.CacheStatus
+}
+
+// fileSize returns the size of the file at path.
+func fileSize(t *testing.T, path string) int64 {
+	t.Helper()
+
+	info, err := os.Stat(path)
+	if err != nil {
+		t.Fatalf("stat %s: %v", path, err)
+	}
+
+	return info.Size()
+}
+
+// truncateFile cuts the file at path down to size bytes.
+func truncateFile(t *testing.T, path string, size int64) {
+	t.Helper()
+
+	t.Logf("cutting %s from %d to %d bytes",
+		filepath.Base(path), fileSize(t, path), size)
+
+	err := os.Truncate(path, size)
+	if err != nil {
+		t.Fatalf("truncating %s: %v", path, err)
+	}
+}
+
+// cachedSourcePath returns the path of the cached source file req uses.
+func cachedSourcePath(t *testing.T, svc *Service, req *ImageRequest) string {
+	t.Helper()
+
+	contentHash, _, err := svc.cache.LookupSource(t.Context(), req)
+	if err != nil || contentHash == "" {
+		t.Fatalf("LookupSource() = %q, %v; want the cached source", contentHash, err)
+	}
+
+	return svc.cache.srcContent.hashToPath(contentHash)
+}
+
+// reconcileAsAtStartup runs the cache's reconciliation pass, as pixa does
+// when it starts, and returns what the pass logged.
+func reconcileAsAtStartup(t *testing.T, cache *Cache) string {
+	t.Helper()
+
+	var logBuf bytes.Buffer
+
+	cache.log = slog.New(slog.NewJSONHandler(&logBuf, nil))
+
+	err := cache.reconcileAccounting(t.Context())
+	if err != nil {
+		t.Fatalf("reconcileAccounting() error = %v", err)
+	}
+
+	t.Logf("reconciliation log: %s", logBuf.String())
+
+	return logBuf.String()
+}
+
+// assertWarningNames checks that logs hold a warning naming the file at
+// path.
+func assertWarningNames(t *testing.T, logs, path string) {
+	t.Helper()
+
+	for line := range strings.Lines(logs) {
+		if strings.Contains(line, `"level":"WARN"`) &&
+			strings.Contains(line, `"path":"`+path+`"`) {
+			return
+		}
+	}
+
+	t.Errorf("reconciliation logged no warning naming %s", path)
+}
+
+// TestService_Get_FetchesAgainForEmptyCachedSource empties the cached source
+// file, as a crash or a restored snapshot could, and runs the reconciliation
+// pass, which must remove it with a warning naming it. It then asks for two
+// new sizes of the source: the first must fetch it again and cache it whole,
+// so that the second is made from the cache without a third fetch.
+func TestService_Get_FetchesAgainForEmptyCachedSource(t *testing.T) {
+	t.Parallel()
+
+	svc, fixtures := SetupTestService(t)
+
+	getImage(t, svc, photoVariant(fixtures, 85, FitCover))
+
+	path := cachedSourcePath(t, svc, photoVariant(fixtures, 85, FitCover))
+	wholeSize := fileSize(t, path)
+
+	truncateFile(t, path, 0)
+	assertWarningNames(t, reconcileAsAtStartup(t, svc.cache), path)
+
+	getImage(t, svc, photoVariant(fixtures, 40, FitCover))
+	getImage(t, svc, photoVariant(fixtures, 30, FitCover))
+
+	fetches := readCacheStatsCounters(t, svc.cache).upstreamFetchCount
+	t.Logf("upstream fetches = %d", fetches)
+
+	if fetches != 2 {
+		t.Errorf("upstream fetches = %d, want 2: the empty cached source "+
+			"was kept and fetched again on every new size", fetches)
+	}
+
+	size := fileSize(t, path)
+	if size != wholeSize {
+		t.Errorf("cached source is %d bytes, want %d", size, wholeSize)
+	}
+}
+
+// TestService_Get_FetchesAgainForPartialCachedSource cuts the cached source
+// file to half its size, as a crash or a restored snapshot could, and runs
+// the reconciliation pass, which must remove it with a warning naming it.
+// It then asks for two new sizes of the source: the first must fetch it
+// again rather than fail to decode the partial file, and the second is
+// made from the cache.
+func TestService_Get_FetchesAgainForPartialCachedSource(t *testing.T) {
+	t.Parallel()
+
+	svc, fixtures := SetupTestService(t)
+
+	getImage(t, svc, photoVariant(fixtures, 85, FitCover))
+
+	path := cachedSourcePath(t, svc, photoVariant(fixtures, 85, FitCover))
+	wholeSize := fileSize(t, path)
+
+	truncateFile(t, path, wholeSize/2)
+	assertWarningNames(t, reconcileAsAtStartup(t, svc.cache), path)
+
+	getImage(t, svc, photoVariant(fixtures, 40, FitCover))
+	getImage(t, svc, photoVariant(fixtures, 30, FitCover))
+
+	fetches := readCacheStatsCounters(t, svc.cache).upstreamFetchCount
+	t.Logf("upstream fetches = %d", fetches)
+
+	if fetches != 2 {
+		t.Errorf("upstream fetches = %d, want 2", fetches)
+	}
+
+	size := fileSize(t, path)
+	if size != wholeSize {
+		t.Errorf("cached source is %d bytes, want %d", size, wholeSize)
+	}
+}
+
+// assertConvertedAgain caches a variant of the photo, cuts its file to size
+// bytes and runs the reconciliation pass, which must remove it with a
+// warning naming it. It then checks that Get converts the image again
+// instead of serving the file, and that the next Get serves the whole
+// variant from the cache.
+func assertConvertedAgain(t *testing.T, size int64) {
+	t.Helper()
+
+	svc, fixtures := SetupTestService(t)
+	req := photoVariant(fixtures, 85, FitCover)
+
+	want, _ := getImage(t, svc, req)
+
+	path := svc.cache.variants.keyToPath(CacheKey(req))
+
+	truncateFile(t, path, size)
+	assertWarningNames(t, reconcileAsAtStartup(t, svc.cache), path)
+
+	got, status := getImage(t, svc, req)
+	t.Logf("after the cut: %d bytes, %s", len(got), status)
+
+	if status != CacheMiss || !bytes.Equal(got, want) {
+		t.Errorf("after the cut: Get() = %d bytes, %s; want the %d bytes "+
+			"converted again, %s", len(got), status, len(want), CacheMiss)
+	}
+
+	got, status = getImage(t, svc, req)
+	t.Logf("next request: %d bytes, %s", len(got), status)
+
+	if status != CacheHit || !bytes.Equal(got, want) {
+		t.Errorf("next request: Get() = %d bytes, %s; want the %d bytes, %s",
+			len(got), status, len(want), CacheHit)
+	}
+}
+
+// TestService_Get_ConvertsAgainForEmptyCachedVariant empties a cached
+// variant file, as a crash or a restored snapshot could, and checks that it
+// is converted again rather than served empty.
+func TestService_Get_ConvertsAgainForEmptyCachedVariant(t *testing.T) {
+	t.Parallel()
+
+	assertConvertedAgain(t, 0)
+}
+
+// TestService_Get_ConvertsAgainForPartialCachedVariant cuts a cached
+// variant file to ten bytes, as a crash or a restored snapshot could, and
+// checks that it is converted again rather than served cut short.
+func TestService_Get_ConvertsAgainForPartialCachedVariant(t *testing.T) {
+	t.Parallel()
+
+	assertConvertedAgain(t, 10)
+}
+
+// TestReconciliationRemovesEmptyVariantFileWithoutRow checks that the
+// reconciliation pass removes an empty variant file that has no
+// accounting row, with a warning naming it, instead of adding a row for
+// it that would have it served empty.
+func TestReconciliationRemovesEmptyVariantFileWithoutRow(t *testing.T) {
+	t.Parallel()
+
+	cache, _ := newEvictionTestCache(t, 1<<30)
+
+	_, err := cache.variants.Store(testVariantKeyOne, bytes.NewReader(nil),
+		testContentTypeWebP)
+	if err != nil {
+		t.Fatalf("failed to store an empty variant file: %v", err)
+	}
+
+	logs := reconcileAsAtStartup(t, cache)
+
+	assertWarningNames(t, logs, cache.variants.keyToPath(testVariantKeyOne))
+
+	if cache.variants.Exists(testVariantKeyOne) {
+		t.Error("the empty variant file is still on disk")
+	}
+
+	rows := countRows(t, cache,
+		`SELECT COUNT(*) FROM variant_content WHERE cache_key = ?`,
+		string(testVariantKeyOne))
+	if rows != 0 {
+		t.Errorf("the empty variant file got %d accounting rows, want 0", rows)
+	}
+}
+
+// TestVariantStorage_StoreReplacesMetaFile stores a variant over one
+// already stored and checks that Store puts a new .meta file in place of
+// the old one, as it does the image, rather than writing into the old
+// file, which a crash could leave empty. The old .meta file is given a
+// second name before the store, so that it can be told apart from the new
+// one after it.
+func TestVariantStorage_StoreReplacesMetaFile(t *testing.T) {
+	t.Parallel()
+
+	storage, key := storeTestVariant(t)
+	metaPath := storage.keyToPath(key) + variantMetaSuffix
+	oldMetaPath := metaPath + ".old"
+
+	err := os.Link(metaPath, oldMetaPath)
+	if err != nil {
+		t.Fatalf("giving the .meta file a second name: %v", err)
+	}
+
+	_, err = storage.Store(key, bytes.NewReader([]byte("new variant data")),
+		testContentTypeJPEG)
+	if err != nil {
+		t.Fatalf("Store() error = %v", err)
+	}
+
+	oldMeta, err := os.Stat(oldMetaPath)
+	if err != nil {
+		t.Fatalf("stat of the old .meta file: %v", err)
+	}
+
+	newMeta, err := os.Stat(metaPath)
+	if err != nil {
+		t.Fatalf("stat of the new .meta file: %v", err)
+	}
+
+	if os.SameFile(oldMeta, newMeta) {
+		t.Error("Store() wrote into the old .meta file instead of putting " +
+			"a new one in its place")
+	}
+
+	reader, _, contentType, err := storage.LoadWithMeta(key)
+	if err != nil {
+		t.Fatalf("LoadWithMeta() error = %v", err)
+	}
+
+	_ = reader.Close()
+
+	t.Logf("content type after the store: %s", contentType)
+
+	if contentType != testContentTypeJPEG {
+		t.Errorf("content type = %q, want %q", contentType, testContentTypeJPEG)
+	}
+}
+
+// TestContentStorage_StoreHashedReplacesFileOfAnotherSize checks that a
+// store over a file of another size at the content's path, such as a
+// partial file a crash left, replaces it with the content.
+func TestContentStorage_StoreHashedReplacesFileOfAnotherSize(t *testing.T) {
+	t.Parallel()
+
+	storage, err := NewContentStorage(t.TempDir())
+	if err != nil {
+		t.Fatalf("NewContentStorage() error = %v", err)
+	}
+
+	content := []byte("the whole source image")
+	sum := sha256.Sum256(content)
+	hash := ContentHash(hex.EncodeToString(sum[:]))
+	path := storage.hashToPath(hash)
+
+	err = os.MkdirAll(filepath.Dir(path), StorageDirPerm)
+	if err != nil {
+		t.Fatalf("creating the content's directory: %v", err)
+	}
+
+	err = os.WriteFile(path, content[:5], StorageFilePerm)
+	if err != nil {
+		t.Fatalf("writing a partial file: %v", err)
+	}
+
+	_, err = storage.StoreHashed(hash, content)
+	if err != nil {
+		t.Fatalf("StoreHashed() error = %v", err)
+	}
+
+	stored, err := storage.Load(hash)
+	if err != nil {
+		t.Fatalf("Load() error = %v", err)
+	}
+
+	defer func() { _ = stored.Close() }()
+
+	got, err := io.ReadAll(stored)
+	if err != nil {
+		t.Fatalf("reading the stored content: %v", err)
+	}
+
+	if !bytes.Equal(got, content) {
+		t.Errorf("stored file = %q, want %q", got, content)
+	}
+}

internal/imgcache/storage.go

@@ -80,8 +80,8 @@ func (s *ContentStorage) Store(r io.Reader) (ContentHash, int64, error) {
 // from hash, without recomputing it. Callers that already know the
 // hash before writing (e.g. because they must hold a hash-keyed lock
 // across the whole store operation) use this instead of Store. Like
-// Store, it is idempotent: content already on disk at that path is
+// Store, it is idempotent: a file of the content's size already at that
-// left untouched.
+// path is left untouched, and a file of another size is replaced.
 func (s *ContentStorage) StoreHashed(
 	hash ContentHash, data []byte,
 ) (int64, error) {
@@ -152,28 +152,35 @@ func (s *ContentStorage) Exists(hash ContentHash) bool {
 	return err == nil
 }
 
-// writeIfAbsent writes data to the path derived from hash, unless
+// writeIfAbsent writes data with writeFileSynced to the path derived
-// content already exists there, via a temp-file-plus-rename so
+// from hash, unless a file of the data's size is already there.
-// concurrent readers never observe a partial file.
 func (s *ContentStorage) writeIfAbsent(hash ContentHash, data []byte) error {
 	// Build path: <basedir>/<ab>/<cd>/<hash>
 	path := s.hashToPath(hash)
 
-	// Check if already exists
+	// A file of another size there is damaged, and is replaced
-	_, statErr := os.Stat(path)
+	info, statErr := os.Stat(path)
-	if statErr == nil {
+	if statErr == nil && info.Size() == int64(len(data)) {
 		return nil
 	}
 
-	// Create directory structure
+	err := os.MkdirAll(filepath.Dir(path), StorageDirPerm)
-	dir := filepath.Dir(path)
-
-	err := os.MkdirAll(dir, StorageDirPerm)
 	if err != nil {
 		return fmt.Errorf("failed to create directory: %w", err)
 	}
 
-	// Write to temp file first, then rename for atomicity
+	return writeFileSynced(path, data)
+}
+
+// writeFileSynced writes data to path through a temp file in the same
+// directory: the temp file is synced to disk, renamed to path, and the
+// directory synced after, so that neither a concurrent reader nor a
+// crash finds a partial file under path. When the directory sync fails,
+// the file is removed from path, so that a failed write leaves no file
+// there.
+func writeFileSynced(path string, data []byte) error {
+	dir := filepath.Dir(path)
+
 	tmpFile, err := os.CreateTemp(dir, ".tmp-*")
 	if err != nil {
 		return fmt.Errorf("failed to create temp file: %w", err)
@@ -181,18 +188,20 @@ func (s *ContentStorage) writeIfAbsent(hash ContentHash, data []byte) error {
 
 	tmpPath := tmpFile.Name()
 
-	// Each failure path below unlinks the temp file explicitly. This
-	// replaces a deferred cleanup that read a named result, which the
-	// canonical config does not permit; the set of paths that remove
-	// tmpPath, and the order relative to Close, is unchanged. This
-	// mirrors how MetadataStorage.Store and VariantStorage.Store below
-	// already express the same cleanup.
 	_, err = tmpFile.Write(data)
 	if err != nil {
 		_ = tmpFile.Close()
 		_ = os.Remove(tmpPath)
 
-		return fmt.Errorf("failed to write content: %w", err)
+		return fmt.Errorf("failed to write temp file: %w", err)
+	}
+
+	err = tmpFile.Sync()
+	if err != nil {
+		_ = tmpFile.Close()
+		_ = os.Remove(tmpPath)
+
+		return fmt.Errorf("failed to sync temp file: %w", err)
 	}
 
 	err = tmpFile.Close()
@@ -202,7 +211,6 @@ func (s *ContentStorage) writeIfAbsent(hash ContentHash, data []byte) error {
 		return fmt.Errorf("failed to close temp file: %w", err)
 	}
 
-	// Atomic rename
 	err = os.Rename(filepath.Clean(tmpPath), filepath.Clean(path))
 	if err != nil {
 		_ = os.Remove(tmpPath)
@@ -210,6 +218,31 @@ func (s *ContentStorage) writeIfAbsent(hash ContentHash, data []byte) error {
 		return fmt.Errorf("failed to rename temp file: %w", err)
 	}
 
+	err = syncDirectory(dir)
+	if err != nil {
+		_ = os.Remove(path)
+
+		return err
+	}
+
+	return nil
+}
+
+// syncDirectory syncs the directory dir to disk, so that a file just
+// renamed into it is still there after a crash.
+func syncDirectory(dir string) error {
+	d, err := os.Open(filepath.Clean(dir))
+	if err != nil {
+		return fmt.Errorf("failed to open directory: %w", err)
+	}
+
+	err = d.Sync()
+	_ = d.Close()
+
+	if err != nil {
+		return fmt.Errorf("failed to sync directory: %w", err)
+	}
+
 	return nil
 }
 
@@ -427,7 +460,9 @@ func NewVariantStorage(baseDir string, log *slog.Logger) (*VariantStorage, error
 	return &VariantStorage{baseDir: baseDir, log: log}, nil
 }
 
-// Store writes content and metadata to storage at the given key.
+// Store writes content and metadata to storage at the given key, each
+// with writeFileSynced. The .meta file is written first, so that a crash
+// cannot leave the content in place without it.
 func (s *VariantStorage) Store(
 	key VariantKey, r io.Reader, contentType string,
 ) (int64, error) {
@@ -440,46 +475,11 @@ func (s *VariantStorage) Store(
 	path := s.keyToPath(key)
 	metaPath := path + ".meta"
 
-	// Create directory structure
+	err = os.MkdirAll(filepath.Dir(path), StorageDirPerm)
-	dir := filepath.Dir(path)
-
-	err = os.MkdirAll(dir, StorageDirPerm)
 	if err != nil {
 		return 0, fmt.Errorf("failed to create directory: %w", err)
 	}
 
-	// Write content to temp file first, then rename for atomicity
-	tmpFile, err := os.CreateTemp(dir, ".tmp-*")
-	if err != nil {
-		return 0, fmt.Errorf("failed to create temp file: %w", err)
-	}
-
-	tmpPath := tmpFile.Name()
-
-	_, err = tmpFile.Write(data)
-	if err != nil {
-		_ = tmpFile.Close()
-		_ = os.Remove(tmpPath)
-
-		return 0, fmt.Errorf("failed to write content: %w", err)
-	}
-
-	err = tmpFile.Close()
-	if err != nil {
-		_ = os.Remove(tmpPath)
-
-		return 0, fmt.Errorf("failed to close temp file: %w", err)
-	}
-
-	// Atomic rename content
-	err = os.Rename(filepath.Clean(tmpPath), filepath.Clean(path))
-	if err != nil {
-		_ = os.Remove(tmpPath)
-
-		return 0, fmt.Errorf("failed to rename temp file: %w", err)
-	}
-
-	// Write metadata
 	meta := VariantMeta{
 		ContentType: contentType,
 		Size:        size,
@@ -491,13 +491,19 @@ func (s *VariantStorage) Store(
 		return 0, fmt.Errorf("failed to marshal metadata: %w", err)
 	}
 
-	// Metadata write failure is non-fatal; content is already stored.
+	// Metadata write failure is non-fatal: the content is then served as
-	err = os.WriteFile(metaPath, metaData, StorageFilePerm)
+	// application/octet-stream.
+	err = writeFileSynced(metaPath, metaData)
 	if err != nil {
 		s.log.Warn("failed to write variant metadata sidecar",
 			"path", metaPath, "error", err)
 	}
 
+	err = writeFileSynced(path, data)
+	if err != nil {
+		return 0, err
+	}
+
 	return size, nil
 }
 

internal/server/logout_internal_test.go

@@ -0,0 +1,269 @@
+package server
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"path/filepath"
+	"regexp"
+	"strings"
+	"testing"
+
+	"go.uber.org/fx"
+	"go.uber.org/fx/fxtest"
+
+	"sneak.berlin/go/pixa/internal/config"
+	"sneak.berlin/go/pixa/internal/database"
+	"sneak.berlin/go/pixa/internal/globals"
+	"sneak.berlin/go/pixa/internal/handlers"
+	"sneak.berlin/go/pixa/internal/healthcheck"
+	"sneak.berlin/go/pixa/internal/logger"
+	"sneak.berlin/go/pixa/internal/middleware"
+	"sneak.berlin/go/pixa/internal/session"
+)
+
+// logoutFormPattern extracts the CSRF token rendered into the generator
+// page's logout form.
+var logoutFormPattern = regexp.MustCompile(
+	`(?s)action="/logout".*?name="gorilla\.csrf\.Token" value="([^"]+)"`)
+
+// startPixa starts the components pixad's fx app builds, from cfg, and
+// returns the server with its routes, as newTestServer does.
+func startPixa(t *testing.T, cfg *config.Config) *Server {
+	t.Helper()
+
+	var (
+		h  *handlers.Handlers
+		mw *middleware.Middleware
+	)
+
+	app := fxtest.New(t,
+		fx.Supply(cfg),
+		fx.Provide(
+			globals.New,
+			logger.New,
+			database.New,
+			healthcheck.New,
+			handlers.New,
+			middleware.New,
+		),
+		fx.Populate(&h, &mw),
+	)
+	app.RequireStart()
+	t.Cleanup(app.RequireStop)
+
+	s := &Server{config: cfg, mw: mw, h: h}
+	s.SetupRoutes()
+
+	return s
+}
+
+// logIn logs in to s with the signing key, as postLogin does, and returns
+// the session cookie it sets.
+func logIn(t *testing.T, s *Server) *http.Cookie {
+	t.Helper()
+
+	rec := postLogin(t, s, firstClient, "", testSigningKey)
+
+	for _, c := range rec.Result().Cookies() {
+		if c.Name == session.CookieName {
+			return c
+		}
+	}
+
+	t.Fatal("logging in set no session cookie")
+
+	return nil
+}
+
+// opensGeneratorPage reports whether GET / with cookie shows s's generator
+// page, which only a valid login session opens.
+func opensGeneratorPage(t *testing.T, s *Server, cookie *http.Cookie) bool {
+	t.Helper()
+
+	req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil)
+	req.AddCookie(cookie)
+
+	rec := httptest.NewRecorder()
+	s.ServeHTTP(rec, req)
+
+	return rec.Code == http.StatusOK &&
+		strings.Contains(rec.Body.String(), `action="/generate"`)
+}
+
+// postLogout loads the generator page with GET / and sessionCookie and
+// submits its logout form with POST /logout, as a browser does.
+func postLogout(
+	t *testing.T, s *Server, sessionCookie *http.Cookie,
+) *httptest.ResponseRecorder {
+	t.Helper()
+
+	req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil)
+	req.AddCookie(sessionCookie)
+
+	page := httptest.NewRecorder()
+	s.ServeHTTP(page, req)
+
+	match := logoutFormPattern.FindStringSubmatch(page.Body.String())
+	if match == nil {
+		t.Fatal("the generator page has no logout form with a CSRF token")
+	}
+
+	form := url.Values{"gorilla.csrf.Token": {match[1]}}
+	req = httptest.NewRequestWithContext(t.Context(), http.MethodPost,
+		"/logout", strings.NewReader(form.Encode()))
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	req.AddCookie(sessionCookie)
+
+	for _, c := range page.Result().Cookies() {
+		req.AddCookie(c)
+	}
+
+	rec := httptest.NewRecorder()
+	s.ServeHTTP(rec, req)
+
+	return rec
+}
+
+// TestLogoutWithoutCSRFTokenEndsNoSession verifies that a request to /logout
+// carrying the session cookie but not the logout form's CSRF token ends no
+// session: GET /logout, as an image on another page of the same site makes a
+// browser send, answers 405, and POST /logout without the token answers 403.
+func TestLogoutWithoutCSRFTokenEndsNoSession(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		method string
+		want   int
+	}{
+		{http.MethodGet, http.StatusMethodNotAllowed},
+		{http.MethodPost, http.StatusForbidden},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.method, func(t *testing.T) {
+			t.Parallel()
+
+			s := newTestServer(t)
+			cookie := logIn(t, s)
+
+			req := httptest.NewRequestWithContext(
+				t.Context(), tc.method, "/logout", nil)
+			req.AddCookie(cookie)
+
+			rec := httptest.NewRecorder()
+			s.ServeHTTP(rec, req)
+			t.Logf("%s /logout without the CSRF token: status %d",
+				tc.method, rec.Code)
+
+			if rec.Code != tc.want {
+				t.Errorf("%s /logout without the CSRF token status = %d, "+
+					"want %d", tc.method, rec.Code, tc.want)
+			}
+
+			if !opensGeneratorPage(t, s, cookie) {
+				t.Errorf("%s /logout without the CSRF token ended the "+
+					"login session", tc.method)
+			}
+		})
+	}
+}
+
+// TestLoginAfterLogoutOpensGeneratorPage verifies that after a logout from
+// the generator page a copy of the session cookie taken before it is
+// refused, and that a login after it opens the generator page.
+func TestLoginAfterLogoutOpensGeneratorPage(t *testing.T) {
+	t.Parallel()
+
+	s := newTestServer(t)
+
+	copied := logIn(t, s)
+	if !opensGeneratorPage(t, s, copied) {
+		t.Fatal("logging in did not open the generator page")
+	}
+
+	logout := postLogout(t, s, copied)
+	if logout.Code != http.StatusSeeOther {
+		t.Fatalf("POST /logout status = %d, want %d",
+			logout.Code, http.StatusSeeOther)
+	}
+
+	if opensGeneratorPage(t, s, copied) {
+		t.Error("a cookie copied before the logout still opens the " +
+			"generator page")
+	}
+
+	opens := opensGeneratorPage(t, s, logIn(t, s))
+	t.Logf("a login after the logout opens the generator page: %v", opens)
+
+	if !opens {
+		t.Error("a login after the logout does not open the generator page")
+	}
+}
+
+// TestLogoutOutlastsRestart verifies that a copy of the session cookie taken
+// before a logout is still refused by a pixa started again on the same state
+// directory, as pixad is after a restart.
+func TestLogoutOutlastsRestart(t *testing.T) {
+	t.Parallel()
+
+	s := newTestServer(t)
+
+	copied := logIn(t, s)
+	if !opensGeneratorPage(t, s, copied) {
+		t.Fatal("logging in did not open the generator page")
+	}
+
+	logout := postLogout(t, s, copied)
+	if logout.Code != http.StatusSeeOther {
+		t.Fatalf("POST /logout status = %d, want %d",
+			logout.Code, http.StatusSeeOther)
+	}
+
+	restarted := startPixa(t, s.config)
+
+	opens := opensGeneratorPage(t, restarted, copied)
+	t.Logf("after the restart, the copied cookie opens the generator page: %v",
+		opens)
+
+	if opens {
+		t.Error("after a restart, a cookie copied before the logout " +
+			"still opens the generator page")
+	}
+}
+
+// TestLostStateDirectoryEndsEveryLoginSession verifies that a pixa started on
+// an empty state directory, as pixad is after a restart that lost its state
+// directory, refuses a session cookie from before, and that a login on it
+// opens the generator page.
+func TestLostStateDirectoryEndsEveryLoginSession(t *testing.T) {
+	t.Parallel()
+
+	s := newTestServer(t)
+
+	before := logIn(t, s)
+	if !opensGeneratorPage(t, s, before) {
+		t.Fatal("logging in did not open the generator page")
+	}
+
+	emptyStateDir := *s.config
+	emptyStateDir.StateDir = t.TempDir()
+	emptyStateDir.DBURL = "file:" +
+		filepath.Join(emptyStateDir.StateDir, "state.sqlite3")
+
+	restarted := startPixa(t, &emptyStateDir)
+
+	opens := opensGeneratorPage(t, restarted, before)
+	t.Logf("on an empty state directory, the cookie from before opens the "+
+		"generator page: %v", opens)
+
+	if opens {
+		t.Error("on an empty state directory, a cookie from before still " +
+			"opens the generator page")
+	}
+
+	if !opensGeneratorPage(t, restarted, logIn(t, restarted)) {
+		t.Error("a login on an empty state directory does not open the " +
+			"generator page")
+	}
+}

internal/server/routes.go

@@ -69,10 +69,9 @@ func (s *Server) SetupRoutes() {
 		r.With(s.mw.RateLimit(LoginAttemptsPerMinute, time.Minute, nil)).
 			Post("/", s.h.HandleRoot())
 		r.Post("/generate", s.h.HandleGenerateURL())
+		r.Post("/logout", s.h.HandleLogout())
 	})
 
-	s.router.Get("/logout", s.h.HandleLogout())
-
 	// Image routes, the only ones that send CORS headers, as pages on other
 	// sites read them. They are a subrouter rather than a group: a group's
 	// middleware runs only for a request that matches one of its routes,

internal/session/session.go

@@ -1,9 +1,16 @@
-// Package session provides encrypted session cookie management.
+// Package session provides encrypted session cookie management, and keeps in
+// the database a random value that each logout replaces, so that a logout
+// ends every session made before it.
 package session
 
 import (
+	"context"
+	"crypto/rand"
+	"database/sql"
 	"errors"
+	"fmt"
 	"net/http"
+	"sync"
 	"time"
 
 	"github.com/gorilla/securecookie"
@@ -32,14 +39,36 @@ type Data struct {
 	Authenticated bool      `json:"auth"`
 	CreatedAt     time.Time `json:"created"`
 	ExpiresAt     time.Time `json:"expires"`
+
+	// LogoutValue is the Manager's random value, which each logout
+	// replaces, when the session was made.
+	LogoutValue string `json:"logoutValue"`
 }
 
 // Manager handles session creation and validation using encrypted cookies.
+//
+// Every login is made with the one signing key, so a logout ends every
+// session made before it, in every browser. The Manager keeps a random
+// value, which each logout replaces: a session carries the value current at
+// its login, and a session carrying another value is refused. The check for
+// a logout compares no time, so a login after a logout works whatever the
+// host's clock does; the session's expiry is still checked against the
+// clock. The Manager keeps the value in memory, so checking a session reads
+// no database; a Manager from NewManagerWithDatabase also saves it in the
+// database, so that it outlasts a restart.
 type Manager struct {
 	sc *securecookie.SecureCookie
+	db *sql.DB
+
+	// mu guards logoutValue, and is held while a logout is saved, so that
+	// the database and logoutValue agree.
+	mu          sync.Mutex
+	logoutValue string
 }
 
 // NewManager creates a session manager with keys derived from the signing key.
+// It makes a new random value and keeps it in memory only, so a restart ends
+// every session; NewManagerWithDatabase keeps the value in the database.
 //
 // Session cookies always carry the Secure, HttpOnly, and SameSite=Strict
 // attributes; this cannot be configured. Browsers treat http://localhost as a
@@ -63,10 +92,44 @@ func NewManager(signingKey string) (*Manager, error) {
 	sc.MaxAge(int(SessionTTL.Seconds()))
 
 	return &Manager{
-		sc: sc,
+		sc:          sc,
+		logoutValue: rand.Text(),
 	}, nil
 }
 
+// NewManagerWithDatabase creates a session manager like NewManager that keeps
+// its random value in db, which must have pixa's schema: it saves the value
+// NewManager made when db has none, and otherwise starts from the one saved
+// there.
+func NewManagerWithDatabase(
+	ctx context.Context, signingKey string, db *sql.DB,
+) (*Manager, error) {
+	m, err := NewManager(signingKey)
+	if err != nil {
+		return nil, err
+	}
+
+	_, err = db.ExecContext(ctx,
+		"INSERT OR IGNORE INTO logout (id, logout_value) VALUES (1, ?)",
+		m.logoutValue)
+	if err != nil {
+		return nil, fmt.Errorf(
+			"failed to save the random value in the logout table: %w", err)
+	}
+
+	err = db.QueryRowContext(ctx,
+		"SELECT logout_value FROM logout WHERE id = 1",
+	).Scan(&m.logoutValue)
+	if err != nil {
+		return nil, fmt.Errorf(
+			"failed to read the random value in the logout table: %w", err)
+	}
+
+	m.db = db
+
+	return m, nil
+}
+
 // CreateSession creates a new authenticated session and sets the cookie.
 func (m *Manager) CreateSession(w http.ResponseWriter) error {
 	now := time.Now()
@@ -74,6 +137,7 @@ func (m *Manager) CreateSession(w http.ResponseWriter) error {
 		Authenticated: true,
 		CreatedAt:     now,
 		ExpiresAt:     now.Add(SessionTTL),
+		LogoutValue:   m.currentLogoutValue(),
 	}
 
 	encoded, err := m.sc.Encode(CookieName, data)
@@ -122,9 +186,30 @@ func (m *Manager) ValidateSession(r *http.Request) (*Data, error) {
 		return nil, ErrInvalidSession
 	}
 
+	// A logout since the session was made has ended it.
+	if data.LogoutValue != m.currentLogoutValue() {
+		return nil, ErrInvalidSession
+	}
+
 	return &data, nil
 }
 
+// Logout removes the session cookie and, when r has a valid session, ends
+// every session made until now. A request without one ends no session, so
+// someone who has not logged in cannot end the sessions of those who have.
+func (m *Manager) Logout(w http.ResponseWriter, r *http.Request) error {
+	if m.IsAuthenticated(r) {
+		err := m.saveLogout(r.Context())
+		if err != nil {
+			return err
+		}
+	}
+
+	m.ClearSession(w)
+
+	return nil
+}
+
 // ClearSession removes the session cookie.
 func (m *Manager) ClearSession(w http.ResponseWriter) {
 	http.SetCookie(w, &http.Cookie{
@@ -145,3 +230,32 @@ func (m *Manager) IsAuthenticated(r *http.Request) bool {
 
 	return err == nil && data != nil && data.Authenticated
 }
+
+// saveLogout replaces the random value with a new one, saving it in the
+// database first when the Manager has one.
+func (m *Manager) saveLogout(ctx context.Context) error {
+	value := rand.Text()
+
+	m.mu.Lock()
+	defer m.mu.Unlock()
+
+	if m.db != nil {
+		_, err := m.db.ExecContext(ctx,
+			"UPDATE logout SET logout_value = ? WHERE id = 1", value)
+		if err != nil {
+			return fmt.Errorf("failed to save the logout: %w", err)
+		}
+	}
+
+	m.logoutValue = value
+
+	return nil
+}
+
+// currentLogoutValue returns the Manager's random value.
+func (m *Manager) currentLogoutValue() string {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+
+	return m.logoutValue
+}

internal/session/session_logout_test.go

@@ -0,0 +1,148 @@
+package session_test
+
+import (
+	"database/sql"
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"sneak.berlin/go/pixa/internal/database"
+	"sneak.berlin/go/pixa/internal/session"
+)
+
+const logoutTestSigningKey = "test-signing-key-12345"
+
+// newTestDatabase opens an empty in-memory database with pixa's schema. It
+// has one connection, as pixad's has: each connection to ":memory:" would
+// open a database of its own.
+func newTestDatabase(t *testing.T) *sql.DB {
+	t.Helper()
+
+	db, err := sql.Open("sqlite", ":memory:")
+	if err != nil {
+		t.Fatalf("sql.Open() error = %v", err)
+	}
+
+	db.SetMaxOpenConns(1)
+	t.Cleanup(func() { _ = db.Close() })
+
+	err = database.ApplyMigrations(t.Context(), db, nil)
+	if err != nil {
+		t.Fatalf("ApplyMigrations() error = %v", err)
+	}
+
+	return db
+}
+
+// createSessionCookie logs in with mgr and returns the session cookie it
+// sets.
+func createSessionCookie(t *testing.T, mgr *session.Manager) *http.Cookie {
+	t.Helper()
+
+	rec := httptest.NewRecorder()
+
+	err := mgr.CreateSession(rec)
+	if err != nil {
+		t.Fatalf("CreateSession() error = %v", err)
+	}
+
+	for _, c := range rec.Result().Cookies() {
+		if c.Name == session.CookieName {
+			return c
+		}
+	}
+
+	t.Fatalf("CreateSession() did not set a %q cookie", session.CookieName)
+
+	return nil
+}
+
+// requestWithCookie returns a GET request carrying cookie, or no cookie when
+// cookie is nil.
+func requestWithCookie(t *testing.T, cookie *http.Cookie) *http.Request {
+	t.Helper()
+
+	req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil)
+	if cookie != nil {
+		req.AddCookie(cookie)
+	}
+
+	return req
+}
+
+// TestLogout_EndsEverySessionMadeBeforeIt verifies that after a logout a
+// copy of the session cookie taken before it is refused and a login after it
+// works, and that both still hold for a new Manager on the same database, as
+// after a restart.
+func TestLogout_EndsEverySessionMadeBeforeIt(t *testing.T) {
+	t.Parallel()
+
+	db := newTestDatabase(t)
+
+	mgr, err := session.NewManagerWithDatabase(
+		t.Context(), logoutTestSigningKey, db)
+	if err != nil {
+		t.Fatalf("NewManagerWithDatabase() error = %v", err)
+	}
+
+	copied := createSessionCookie(t, mgr)
+
+	err = mgr.Logout(httptest.NewRecorder(), requestWithCookie(t, copied))
+	if err != nil {
+		t.Fatalf("Logout() error = %v", err)
+	}
+
+	_, err = mgr.ValidateSession(requestWithCookie(t, copied))
+	t.Logf("cookie copied before the logout: ValidateSession() error = %v", err)
+
+	if !errors.Is(err, session.ErrInvalidSession) {
+		t.Errorf("cookie copied before the logout: error = %v, want %v",
+			err, session.ErrInvalidSession)
+	}
+
+	loginAfter := createSessionCookie(t, mgr)
+
+	_, err = mgr.ValidateSession(requestWithCookie(t, loginAfter))
+	if err != nil {
+		t.Errorf("login after the logout: ValidateSession() error = %v", err)
+	}
+
+	restarted, err := session.NewManagerWithDatabase(
+		t.Context(), logoutTestSigningKey, db)
+	if err != nil {
+		t.Fatalf("NewManagerWithDatabase() after restart error = %v", err)
+	}
+
+	if restarted.IsAuthenticated(requestWithCookie(t, copied)) {
+		t.Error("after a restart, a cookie copied before the logout is accepted")
+	}
+
+	if !restarted.IsAuthenticated(requestWithCookie(t, loginAfter)) {
+		t.Error("after a restart, a login made after the logout is refused")
+	}
+}
+
+// TestLogout_WithoutSession_EndsNoSession verifies that a logout from a
+// request without a session cookie ends no session, so someone who has not
+// logged in cannot end the sessions of those who have.
+func TestLogout_WithoutSession_EndsNoSession(t *testing.T) {
+	t.Parallel()
+
+	mgr, err := session.NewManagerWithDatabase(
+		t.Context(), logoutTestSigningKey, newTestDatabase(t))
+	if err != nil {
+		t.Fatalf("NewManagerWithDatabase() error = %v", err)
+	}
+
+	cookie := createSessionCookie(t, mgr)
+
+	err = mgr.Logout(httptest.NewRecorder(), requestWithCookie(t, nil))
+	if err != nil {
+		t.Fatalf("Logout() error = %v", err)
+	}
+
+	if !mgr.IsAuthenticated(requestWithCookie(t, cookie)) {
+		t.Error("a logout without a session cookie ended a session")
+	}
+}

internal/static/style.css

@@ -122,12 +122,20 @@ header {
     margin-bottom: 2rem;
 }
 
-header a {
+/* The logout button, drawn as a link. */
+
+header button {
+    width: auto;
+    margin: 0;
+    padding: 0;
+    background: none;
     font-size: 0.875rem;
     color: #4b5563;
+    text-decoration: underline;
 }
 
-header a:hover {
+header button:hover {
+    background: none;
     color: #1f2937;
 }
 

internal/templates/generator.html

@@ -10,9 +10,12 @@
     <div class="page">
         <header>
             <h1>Pixa URL Generator</h1>
-            <a href="/logout">
+            <form method="POST" action="/logout">
-                Logout
+                {{ .CSRFField }}
-            </a>
+                <button type="submit">
+                    Logout
+                </button>
+            </form>
         </header>
 
         {{if .GeneratedURL}}