Bound shutdown hooks by their stop context (closes #102)

fx hands OnStop a context carrying the application's stop timeout,
and the delivery engine, the retention reaper, and the archive
sweeper all discarded it and called wg.Wait() bare. A worker wedged
inside a delivery target that never returns, or a sweep blocked on a
locked SQLite database, hung the process forever instead of letting
it exit when the timeout expired.

All three now wait through internal/lifecycle.WaitForShutdown, which
selects the drained WaitGroup against the stop context and, on
timeout, logs at error naming the component and returns an error
rather than reporting a clean stop.

Engine.stop also gains the cancel != nil guard its two mirrored
components already had.

README.md

@@ -150,10 +150,9 @@ one runs out first:
 - **Idle expiry** (`SESSION_IDLE_TIMEOUT`, default `24h`) is a sliding
   window. Every authenticated request pushes it forward, so a session
   in continuous use never hits it, while an abandoned one expires a day
-  after its last use. Any non-positive value (`0`, or a negative
+  after its last use. Set it to `0` to disable idle expiry entirely;
-  duration such as `-1s`) disables idle expiry entirely; the absolute
+  the absolute cap below still applies. A set-but-unparseable value
-  cap below still applies. A set-but-unparseable value aborts startup
+  aborts startup rather than silently falling back to the default.
-  rather than silently falling back to the default.
 - **Absolute expiry** is a fixed 7 days from login. Activity does
   **not** extend it: after a week, every session ends and the user
   authenticates again.
@@ -165,11 +164,6 @@ idle window rather than on every request, which means a session may
 expire up to 10% early relative to the user's true last request, but
 never late.
 
-Both clocks are anchored by timestamps stored in the session cookie.
-Sessions issued before this feature existed carry neither, so they are
-treated as expired: upgrading to a build that has it logs every
-existing session out once, and those users sign in again.
-
 #### Invalid values abort startup
 
 The defaults above apply **only** to variables that are unset (or set

internal/database/export_test.go

@@ -46,8 +46,19 @@ func (r *RetentionReaper) ExportStart() {
 }
 
 // ExportStop stops the reaper's background loop for tests.
-func (r *RetentionReaper) ExportStop() {
+func (r *RetentionReaper) ExportStop(ctx context.Context) error {
-	r.stop()
+	return r.stop(ctx)
+}
+
+// ExportWedgeLoop adds a goroutine to the reaper's WaitGroup that
+// never observes cancellation and returns only when release is
+// closed. It stands in for a sweep stuck on a locked database.
+func (r *RetentionReaper) ExportWedgeLoop(
+	release <-chan struct{},
+) {
+	r.wg.Go(func() {
+		<-release
+	})
 }
 
 // ExportSetInterval overrides the sweep interval for tests.

internal/database/retention.go

@@ -10,6 +10,7 @@ import (
 	"go.uber.org/fx"
 	"gorm.io/gorm"
 	"sneak.berlin/go/webhooker/internal/config"
+	"sneak.berlin/go/webhooker/internal/lifecycle"
 	"sneak.berlin/go/webhooker/internal/logger"
 )
 
@@ -62,8 +63,9 @@ func NewRetentionReaper(
 }
 
 // registerHooks wires the reaper's start and stop into the fx
-// lifecycle. The start hook's context is deliberately ignored: see
+// lifecycle. The start hook's context is deliberately ignored (see
-// start for why the sweep loop must not inherit it.
+// start for why the sweep loop must not inherit it); the stop hook's
+// context is honoured (see stop).
 func (r *RetentionReaper) registerHooks(lc fx.Lifecycle) {
 	lc.Append(fx.Hook{
 		//nolint:contextcheck // Not inheriting the hook context is
@@ -73,10 +75,8 @@ func (r *RetentionReaper) registerHooks(lc fx.Lifecycle) {
 
 			return nil
 		},
-		OnStop: func(_ context.Context) error {
+		OnStop: func(ctx context.Context) error {
-			r.stop()
+			return r.stop(ctx)
-
-			return nil
 		},
 	})
 }
@@ -105,15 +105,27 @@ func (r *RetentionReaper) start() {
 	)
 }
 
-func (r *RetentionReaper) stop() {
+// stop cancels the sweep loop's context and waits for it to
+// exit, bounded by the stop hook's context: a sweep wedged on a
+// locked database must not hang the process past fx's stop
+// timeout.
+func (r *RetentionReaper) stop(ctx context.Context) error {
 	r.log.Info("retention reaper stopping")
 
 	if r.cancel != nil {
 		r.cancel()
 	}
 
-	r.wg.Wait()
+	err := lifecycle.WaitForShutdown(
+		ctx, r.log, "retention reaper", &r.wg,
+	)
+	if err != nil {
+		return err
+	}
+
 	r.log.Info("retention reaper stopped")
+
+	return nil
 }
 
 func (r *RetentionReaper) run(ctx context.Context) {

internal/database/retention_lifecycle_test.go

@@ -26,6 +26,13 @@ const (
 	// reaperTestRetentionDays is the retention policy the lifecycle
 	// tests give their webhook.
 	reaperTestRetentionDays = 30
+
+	// reaperWedgeStopTimeout is the stop timeout the wedged-shutdown
+	// test hands OnStop, standing in for fx's StopTimeout. The test
+	// asserts only that the hook returns at all, and allows it
+	// reaperStopTimeout — forty times this budget — to do so, so no
+	// assertion races the wall clock.
+	reaperWedgeStopTimeout = 250 * time.Millisecond
 )
 
 // recordingLifecycle is a minimal fx.Lifecycle that records the
@@ -207,3 +214,59 @@ func TestRetentionReaper_StopHookStopsLoop(t *testing.T) {
 		"a stopped reaper must not sweep anything",
 	)
 }
+
+// TestRetentionReaper_StopHookHonoursStopTimeout is the
+// regression test for a shutdown that could never complete. fx
+// hands OnStop a context carrying the application's stop timeout;
+// an OnStop that discards it and calls wg.Wait() bare hangs the
+// process forever on a sweep blocked on a locked SQLite database
+// — precisely when a bounded shutdown matters most.
+//
+// The wedged goroutine here never observes cancellation, so the
+// hook can only return by honouring its context, and it must say
+// so rather than reporting a clean stop.
+func TestRetentionReaper_StopHookHonoursStopTimeout(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	env := setupRetentionTest(t)
+
+	env.reaper.ExportSetInterval(reaperTestInterval)
+
+	lc := startReaperViaHook(t, env.reaper)
+
+	release := make(chan struct{})
+
+	t.Cleanup(func() { close(release) })
+
+	env.reaper.ExportWedgeLoop(release)
+
+	stopCtx, cancel := context.WithTimeout(
+		context.Background(), reaperWedgeStopTimeout,
+	)
+	defer cancel()
+
+	var stopErr error
+
+	stopped := make(chan struct{})
+
+	go func() {
+		defer close(stopped)
+
+		stopErr = lc.hooks[0].OnStop(stopCtx)
+	}()
+
+	select {
+	case <-stopped:
+	case <-time.After(reaperStopTimeout):
+		t.Fatal(
+			"OnStop did not return: it discarded the stop " +
+				"context and is waiting on a wedged goroutine " +
+				"that will never observe cancellation",
+		)
+	}
+
+	require.ErrorIs(t, stopErr, context.DeadlineExceeded)
+	require.ErrorContains(t, stopErr, "retention reaper")
+}

internal/delivery/archive_sweeper.go

@@ -10,6 +10,7 @@ import (
 	"go.uber.org/fx"
 	"sneak.berlin/go/webhooker/internal/config"
 	"sneak.berlin/go/webhooker/internal/database"
+	"sneak.berlin/go/webhooker/internal/lifecycle"
 	"sneak.berlin/go/webhooker/internal/logger"
 )
 
@@ -67,10 +68,9 @@ func NewArchiveSweeper(
 }
 
 // registerHooks wires the sweeper's start and stop into the fx
-// lifecycle. Both hook contexts are deliberately ignored: see
+// lifecycle. The start hook's context is deliberately ignored
-// start for why the background loop must not inherit the start
+// (see start for why the background loop must not inherit it);
-// hook's context, and stop for why shutdown blocks on the loop
+// the stop hook's context is honoured (see stop).
-// rather than on the stop hook's deadline.
 func (s *ArchiveSweeper) registerHooks(lc fx.Lifecycle) {
 	lc.Append(fx.Hook{
 		//nolint:contextcheck // Not passing the hook context is
@@ -80,10 +80,8 @@ func (s *ArchiveSweeper) registerHooks(lc fx.Lifecycle) {
 
 			return nil
 		},
-		OnStop: func(_ context.Context) error {
+		OnStop: func(ctx context.Context) error {
-			s.stop()
+			return s.stop(ctx)
-
-			return nil
 		},
 	})
 }
@@ -113,15 +111,27 @@ func (s *ArchiveSweeper) start() {
 	)
 }
 
-func (s *ArchiveSweeper) stop() {
+// stop cancels the sweep loop's context and waits for it to
+// exit, bounded by the stop hook's context: a prune wedged on a
+// locked archive must not hang the process past fx's stop
+// timeout.
+func (s *ArchiveSweeper) stop(ctx context.Context) error {
 	s.log.Info("archive sweeper stopping")
 
 	if s.cancel != nil {
 		s.cancel()
 	}
 
-	s.wg.Wait()
+	err := lifecycle.WaitForShutdown(
+		ctx, s.log, "archive sweeper", &s.wg,
+	)
+	if err != nil {
+		return err
+	}
+
 	s.log.Info("archive sweeper stopped")
+
+	return nil
 }
 
 func (s *ArchiveSweeper) run(ctx context.Context) {

internal/delivery/archive_sweeper_test.go

@@ -14,7 +14,6 @@ import (
 	"github.com/google/uuid"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"go.uber.org/fx"
 	"gorm.io/driver/sqlite"
 	"gorm.io/gorm"
 	"gorm.io/gorm/clause"
@@ -226,17 +225,6 @@ func countArchivedRows(path string) (int64, error) {
 	return count, nil
 }
 
-// captureLifecycle is a minimal fx.Lifecycle that records the
-// hooks a component registers, so a test can invoke the real
-// OnStart/OnStop functions with a context of its choosing.
-type captureLifecycle struct {
-	hooks []fx.Hook
-}
-
-func (l *captureLifecycle) Append(h fx.Hook) {
-	l.hooks = append(l.hooks, h)
-}
-
 // TestArchiveSweeper_LoopOutlivesStartHookContext is the
 // regression test for a sweeper that never swept. fx calls
 // OnStart with a context carrying the application's start
@@ -270,7 +258,7 @@ func TestArchiveSweeper_LoopOutlivesStartHookContext(
 
 	// Drive the genuine fx hooks the application registers,
 	// rather than a test-only entry point.
-	lc := &captureLifecycle{}
+	lc := &recordingLifecycle{}
 	env.sweeper.ExportRegisterHooks(lc)
 	require.Len(t, lc.hooks, 1)
 
@@ -924,7 +912,36 @@ func TestArchiveSweeper_StopsCleanly(t *testing.T) {
 	env.sweeper.ExportSetInterval(time.Millisecond)
 	env.sweeper.ExportStart()
 
-	// stop blocks on the loop's WaitGroup, so returning at all
+	// stop blocks on the loop's WaitGroup, so returning without
-	// proves the loop observed the cancellation and exited.
+	// error proves the loop observed the cancellation and exited
-	env.sweeper.ExportStop()
+	// well inside the stop context.
+	require.NoError(
+		t, env.sweeper.ExportStop(context.Background()),
+	)
+}
+
+// TestArchiveSweeper_StopHookHonoursStopTimeout is the sweeper's
+// half of the same shutdown defect the engine and the retention
+// reaper carried: an OnStop that discards its context and waits
+// on the WaitGroup bare hangs the process forever on a prune
+// wedged inside a locked archive.
+func TestArchiveSweeper_StopHookHonoursStopTimeout(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	env := setupSweeperTest(t)
+
+	lc := &recordingLifecycle{}
+	env.sweeper.ExportRegisterHooks(lc)
+	require.Len(t, lc.hooks, 1)
+	require.NoError(t, lc.hooks[0].OnStart(context.Background()))
+
+	release := make(chan struct{})
+
+	t.Cleanup(func() { close(release) })
+
+	env.sweeper.ExportWedgeLoop(release)
+
+	requireStopHookExpires(t, lc.hooks[0], "archive sweeper")
 }

internal/delivery/engine.go

@@ -13,6 +13,7 @@ import (
 	"go.uber.org/fx"
 	"gorm.io/gorm"
 	"sneak.berlin/go/webhooker/internal/database"
+	"sneak.berlin/go/webhooker/internal/lifecycle"
 	"sneak.berlin/go/webhooker/internal/logger"
 )
 
@@ -234,8 +235,9 @@ func (e *Engine) ScheduleRetry(
 }
 
 // registerHooks wires the engine's start and stop into the fx
-// lifecycle. The start hook's context is deliberately ignored:
+// lifecycle. The start hook's context is deliberately ignored
-// see start for why the worker pool must not inherit it.
+// (see start for why the worker pool must not inherit it); the
+// stop hook's context is honoured (see stop).
 func (e *Engine) registerHooks(lc fx.Lifecycle) {
 	lc.Append(fx.Hook{
 		//nolint:contextcheck // Not inheriting the hook context
@@ -245,10 +247,8 @@ func (e *Engine) registerHooks(lc fx.Lifecycle) {
 
 			return nil
 		},
-		OnStop: func(_ context.Context) error {
+		OnStop: func(ctx context.Context) error {
-			e.stop()
+			return e.stop(ctx)
-
-			return nil
 		},
 	})
 }
@@ -289,11 +289,26 @@ func (e *Engine) start() {
 	)
 }
 
-func (e *Engine) stop() {
+// stop cancels the worker pool's context and waits for the pool
+// to drain, bounded by the stop hook's context: a wedged worker
+// must not hang the process past fx's stop timeout.
+func (e *Engine) stop(ctx context.Context) error {
 	e.log.Info("delivery engine stopping")
-	e.cancel()
+
-	e.wg.Wait()
+	if e.cancel != nil {
+		e.cancel()
+	}
+
+	err := lifecycle.WaitForShutdown(
+		ctx, e.log, "delivery engine", &e.wg,
+	)
+	if err != nil {
+		return err
+	}
+
 	e.log.Info("delivery engine stopped")
+
+	return nil
 }
 
 func (e *Engine) worker(ctx context.Context) {

internal/delivery/engine_integration_test.go

@@ -501,7 +501,7 @@ func TestWorkerLifecycle_StartStop(t *testing.T) {
 
 	iWaitForDelivered(t, s.WebhookDB, d.ID)
 
-	s.Engine.ExportStop()
+	require.NoError(t, s.Engine.ExportStop(context.Background()))
 }
 
 // iWaitForDelivered polls until the delivery reaches the
@@ -567,7 +567,7 @@ func TestWorkerLifecycle_ProcessesRetryChannel(
 
 	iWaitForDelivered(t, s.WebhookDB, d.ID)
 
-	s.Engine.ExportStop()
+	require.NoError(t, s.Engine.ExportStop(context.Background()))
 }
 
 // --- processDelivery: unknown target type ---

internal/delivery/engine_lifecycle_test.go

@@ -27,6 +27,13 @@ const (
 	// and a ready deliveryCh are chosen between at random and a
 	// doomed pool still delivers.
 	hookSettleDelay = 250 * time.Millisecond
+
+	// wedgeStopTimeout is the stop timeout a wedged-shutdown test
+	// hands OnStop, standing in for fx's StopTimeout. The test
+	// asserts only that the hook returns at all, and allows it
+	// hookStopTimeout — forty times this budget — to do so, so no
+	// assertion here races the wall clock.
+	wedgeStopTimeout = 250 * time.Millisecond
 )
 
 // recordingLifecycle is a minimal fx.Lifecycle that records the
@@ -40,6 +47,44 @@ func (l *recordingLifecycle) Append(h fx.Hook) {
 	l.hooks = append(l.hooks, h)
 }
 
+// requireStopHookExpires drives hook.OnStop with a stop context
+// that expires while a wedged goroutine is still running, and
+// requires the hook to return the deadline error naming
+// component instead of blocking on the WaitGroup forever.
+func requireStopHookExpires(
+	t *testing.T, hook fx.Hook, component string,
+) {
+	t.Helper()
+
+	stopCtx, cancel := context.WithTimeout(
+		context.Background(), wedgeStopTimeout,
+	)
+	defer cancel()
+
+	var stopErr error
+
+	stopped := make(chan struct{})
+
+	go func() {
+		defer close(stopped)
+
+		stopErr = hook.OnStop(stopCtx)
+	}()
+
+	select {
+	case <-stopped:
+	case <-time.After(hookStopTimeout):
+		t.Fatal(
+			"OnStop did not return: it discarded the stop " +
+				"context and is waiting on a wedged goroutine " +
+				"that will never observe cancellation",
+		)
+	}
+
+	require.ErrorIs(t, stopErr, context.DeadlineExceeded)
+	require.ErrorContains(t, stopErr, component)
+}
+
 // startEngineViaHook drives the genuine fx hooks the application
 // registers for the engine, handing OnStart a context that is
 // already done, and returns only once a pool that inherited that
@@ -197,3 +242,30 @@ func TestEngine_StopHookStopsWorkers(t *testing.T) {
 		"a stopped engine must not deliver anything",
 	)
 }
+
+// TestEngine_StopHookHonoursStopTimeout is the regression test
+// for a shutdown that could never complete. fx hands OnStop a
+// context carrying the application's stop timeout; an OnStop
+// that discards it and calls wg.Wait() bare hangs the process
+// forever on a single worker stuck inside a delivery target that
+// never returns — precisely when a bounded shutdown matters
+// most.
+//
+// The wedged goroutine here never observes cancellation, so the
+// hook can only return by honouring its context, and it must say
+// so rather than reporting a clean stop.
+func TestEngine_StopHookHonoursStopTimeout(t *testing.T) {
+	t.Parallel()
+
+	s := newISetup(t)
+
+	lc := startEngineViaHook(t, s.Engine)
+
+	release := make(chan struct{})
+
+	t.Cleanup(func() { close(release) })
+
+	s.Engine.ExportWedgeWorker(release)
+
+	requireStopHookExpires(t, lc.hooks[0], "delivery engine")
+}

internal/delivery/export_test.go

@@ -216,8 +216,19 @@ func (e *Engine) ExportRegisterHooks(lc fx.Lifecycle) {
 }
 
 // ExportStop exposes stop for testing.
-func (e *Engine) ExportStop() {
+func (e *Engine) ExportStop(ctx context.Context) error {
-	e.stop()
+	return e.stop(ctx)
+}
+
+// ExportWedgeWorker adds a goroutine to the engine's WaitGroup
+// that never observes cancellation and returns only when release
+// is closed. It stands in for a worker stuck inside a delivery
+// target that never returns, which is the only way stop can be
+// made to outlast its context.
+func (e *Engine) ExportWedgeWorker(release <-chan struct{}) {
+	e.wg.Go(func() {
+		<-release
+	})
 }
 
 // ExportDeliveryCh returns the delivery channel.
@@ -518,8 +529,19 @@ func (s *ArchiveSweeper) ExportRegisterHooks(lc fx.Lifecycle) {
 }
 
 // ExportStop stops the sweeper's background loop for tests.
-func (s *ArchiveSweeper) ExportStop() {
+func (s *ArchiveSweeper) ExportStop(ctx context.Context) error {
-	s.stop()
+	return s.stop(ctx)
+}
+
+// ExportWedgeLoop adds a goroutine to the sweeper's WaitGroup
+// that never observes cancellation and returns only when release
+// is closed. It stands in for a prune stuck on a locked archive.
+func (s *ArchiveSweeper) ExportWedgeLoop(
+	release <-chan struct{},
+) {
+	s.wg.Go(func() {
+		<-release
+	})
 }
 
 // ExportSetInterval overrides the sweep interval for tests.

internal/lifecycle/lifecycle.go

@@ -0,0 +1,57 @@
+// Package lifecycle holds helpers shared by the components that
+// register fx start and stop hooks.
+package lifecycle
+
+import (
+	"context"
+	"fmt"
+	"log/slog"
+	"sync"
+)
+
+// WaitForShutdown waits for wg to drain, bounded by ctx.
+//
+// fx hands OnStop a context carrying the application's stop
+// timeout. A bare wg.Wait() discards that deadline, so a single
+// goroutine that never observes cancellation — a delivery target
+// that never returns, a SQLite operation blocked on a lock —
+// hangs the process forever instead of letting it exit when the
+// timeout expires, which is exactly when a clean shutdown matters
+// most.
+//
+// On timeout it logs at error naming component and returns an
+// error: the goroutines are still running, and reporting success
+// would hide an unclean shutdown from the operator. The waiting
+// goroutine outlives this call and exits when (if) wg drains; it
+// holds nothing but the channel it closes.
+func WaitForShutdown(
+	ctx context.Context,
+	log *slog.Logger,
+	component string,
+	wg *sync.WaitGroup,
+) error {
+	done := make(chan struct{})
+
+	go func() {
+		defer close(done)
+
+		wg.Wait()
+	}()
+
+	select {
+	case <-done:
+		return nil
+	case <-ctx.Done():
+		log.Error(
+			"shutdown timed out, goroutines still running",
+			"component", component,
+			"error", ctx.Err(),
+		)
+
+		return fmt.Errorf(
+			"%s: shutdown timed out, "+
+				"goroutines still running: %w",
+			component, ctx.Err(),
+		)
+	}
+}

internal/lifecycle/lifecycle_test.go

@@ -0,0 +1,62 @@
+package lifecycle_test
+
+import (
+	"context"
+	"log/slog"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/lifecycle"
+)
+
+// waitTimeout is the stop budget the timeout case gives a
+// goroutine that never returns. The test's own patience is the
+// go test deadline, so the only thing this value affects is how
+// long the case takes.
+const waitTimeout = 100 * time.Millisecond
+
+func discardLogger() *slog.Logger {
+	return slog.New(slog.DiscardHandler)
+}
+
+func TestWaitForShutdown_DrainedGroup(t *testing.T) {
+	t.Parallel()
+
+	var wg sync.WaitGroup
+
+	wg.Go(func() {})
+
+	require.NoError(
+		t,
+		lifecycle.WaitForShutdown(
+			context.Background(), discardLogger(),
+			"test component", &wg,
+		),
+	)
+}
+
+func TestWaitForShutdown_ContextExpires(t *testing.T) {
+	t.Parallel()
+
+	release := make(chan struct{})
+
+	t.Cleanup(func() { close(release) })
+
+	var wg sync.WaitGroup
+
+	wg.Go(func() { <-release })
+
+	ctx, cancel := context.WithTimeout(
+		context.Background(), waitTimeout,
+	)
+	defer cancel()
+
+	err := lifecycle.WaitForShutdown(
+		ctx, discardLogger(), "test component", &wg,
+	)
+
+	require.ErrorIs(t, err, context.DeadlineExceeded)
+	require.ErrorContains(t, err, "test component")
+}

internal/middleware/middleware.go

@@ -214,11 +214,11 @@ func (s *Middleware) RequireAuth() func(http.Handler) http.Handler {
 			// handler runs, while the headers are still ours to
 			// write.
 			if s.session.Touch(sess) {
-				saveErr := s.session.Save(r, w, sess)
+				err = s.session.Save(r, w, sess)
-				if saveErr != nil {
+				if err != nil {
 					s.log.Error(
 						"auth middleware: failed to refresh session",
-						"error", saveErr,
+						"error", err,
 					)
 				}
 			}

internal/session/codec_test.go

@@ -1,150 +0,0 @@
-package session_test
-
-import (
-	"context"
-	"crypto/hmac"
-	"crypto/sha256"
-	"encoding/base64"
-	"fmt"
-	"net/http"
-	"net/http/httptest"
-	"strings"
-	"testing"
-	"time"
-
-	"github.com/gorilla/sessions"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/session"
-)
-
-// The tests below exercise the securecookie codecs underneath the
-// store and nothing else: Session.Get only decodes, so no server-side
-// expiry check takes part in the result. They exist because
-// NewCookieStore gives its codecs a 30-day max age that assigning
-// store.Options does not override, which would let the codec accept a
-// cookie weeks past the cap the cookie attribute advertises.
-
-// issuedCookie returns a session cookie the store itself wrote.
-func issuedCookie(t *testing.T, s *session.Session) string {
-	t.Helper()
-
-	req := httptest.NewRequestWithContext(
-		context.Background(), http.MethodGet, "/", nil)
-	w := httptest.NewRecorder()
-
-	sess, err := s.Get(req)
-	require.NoError(t, err)
-
-	sess.Values["probe"] = "value"
-	require.NoError(t, s.Save(req, w, sess))
-
-	cookies := w.Result().Cookies()
-	require.Len(t, cookies, 1)
-
-	return cookies[0].Value
-}
-
-// restamp rewrites the timestamp inside an encoded session cookie and
-// re-signs it, yielding the cookie the store would have written at
-// that instant. securecookie stamps the encoding time itself and
-// exposes no seam to move it, so its wire format is reproduced here:
-// the base64url payload is "date|value|mac", where mac is HMAC-SHA256
-// of "name|date|value" under the store's key.
-func restamp(
-	t *testing.T,
-	encoded string,
-	at time.Time,
-) string {
-	t.Helper()
-
-	raw, err := base64.URLEncoding.DecodeString(encoded)
-	require.NoError(t, err)
-
-	parts := strings.SplitN(string(raw), "|", 3)
-	require.Len(t, parts, 3)
-
-	stamped := fmt.Sprintf("%d|%s", at.Unix(), parts[1])
-
-	mac := hmac.New(sha256.New, testKey())
-	_, err = mac.Write([]byte(session.SessionName + "|" + stamped))
-	require.NoError(t, err)
-
-	payload := append([]byte(stamped+"|"), mac.Sum(nil)...)
-
-	return base64.URLEncoding.EncodeToString(payload)
-}
-
-// decodeCookie feeds value back through the store's decode path.
-func decodeCookie(
-	t *testing.T,
-	s *session.Session,
-	value string,
-) (*sessions.Session, error) {
-	t.Helper()
-
-	req := httptest.NewRequestWithContext(
-		context.Background(), http.MethodGet, "/", nil)
-	req.AddCookie(&http.Cookie{
-		Name:     session.SessionName,
-		Value:    value,
-		Path:     "/",
-		HttpOnly: true,
-		Secure:   true,
-		SameSite: http.SameSiteLaxMode,
-	})
-
-	sess, err := s.Get(req)
-	require.NotNil(t, sess)
-
-	return sess, err
-}
-
-func TestCodec_AcceptsCookieInsideAbsoluteCap(t *testing.T) {
-	t.Parallel()
-
-	s := testSession(t)
-
-	sess, err := decodeCookie(t, s, restamp(
-		t,
-		issuedCookie(t, s),
-		time.Now().Add(-(testAbsoluteMaxAge-time.Hour)),
-	))
-	require.NoError(t, err)
-	assert.False(
-		t, sess.IsNew,
-		"a cookie inside the cap must still decode",
-	)
-	assert.Equal(
-		t, "value", sess.Values["probe"],
-		"decoding must yield the values that were saved",
-	)
-}
-
-func TestCodec_RejectsCookiePastAbsoluteCap(t *testing.T) {
-	t.Parallel()
-
-	s := testSession(t)
-
-	sess, err := decodeCookie(t, s, restamp(
-		t,
-		issuedCookie(t, s),
-		time.Now().Add(-(testAbsoluteMaxAge+time.Hour)),
-	))
-	require.Error(
-		t, err,
-		"the codec must refuse a cookie older than the cap",
-	)
-	assert.Contains(
-		t, err.Error(), "expired timestamp",
-		"rejection must come from the codec's age check",
-	)
-	assert.True(
-		t, sess.IsNew,
-		"a cookie past the cap must not populate a session",
-	)
-	assert.Nil(
-		t, sess.Values["probe"],
-		"a cookie past the cap must not yield its values",
-	)
-}

internal/session/export_test.go

@@ -1,10 +0,0 @@
-package session
-
-import "github.com/gorilla/sessions"
-
-// NewStore exposes the production cookie-store constructor so tests
-// exercise the store the application actually runs with, rather than a
-// lookalike assembled in the test.
-func NewStore(key []byte, secure bool) *sessions.CookieStore {
-	return newStore(key, secure)
-}

internal/session/session.go

@@ -100,35 +100,6 @@ type Session struct {
 	now func() time.Time
 }
 
-// cookieOptions returns the cookie attributes used for every session
-// cookie. MaxAge is deliberately left at its zero value: for a store
-// it is set through CookieStore.MaxAge (see newStore), and for a
-// single session it is copied from the store's options.
-func cookieOptions(secure bool) *sessions.Options {
-	return &sessions.Options{
-		Path:     "/",
-		HttpOnly: true,
-		Secure:   secure,
-		SameSite: http.SameSiteLaxMode,
-	}
-}
-
-// newStore builds the session cookie store.
-//
-// The absolute cap MUST be applied with store.MaxAge and not by
-// assigning store.Options.MaxAge. NewCookieStore gives the underlying
-// securecookie codecs a 30-day max age of their own, and assigning
-// Options never touches Codecs -- so a store configured that way still
-// decodes a 30-day-old cookie, leaving the cookie attribute and the
-// codec disagreeing about the same policy. store.MaxAge sets both.
-func newStore(key []byte, secure bool) *sessions.CookieStore {
-	store := sessions.NewCookieStore(key)
-	store.Options = cookieOptions(secure)
-	store.MaxAge(secondsPerDay * sessionMaxAgeDays)
-
-	return store
-}
-
 // New creates a new session manager. The cookie store is
 // initialized during the fx OnStart phase after the database is
 // connected, using a session key that is auto-generated and stored
@@ -171,8 +142,19 @@ func New(
 				)
 			}
 
+			store := sessions.NewCookieStore(keyBytes)
+
+			// Configure cookie options for security
+			store.Options = &sessions.Options{
+				Path:     "/",
+				MaxAge:   secondsPerDay * sessionMaxAgeDays,
+				HttpOnly: true,
+				Secure:   !params.Config.IsDev(),
+				SameSite: http.SameSiteLaxMode,
+			}
+
 			s.key = keyBytes
-			s.store = newStore(keyBytes, !params.Config.IsDev())
+			s.store = store
 			s.log.Info("session manager initialized")
 
 			return nil
@@ -368,8 +350,13 @@ func (s *Session) Regenerate(
 	// Apply the standard session options (the destroyed old
 	// session had MaxAge = -1, which store.New might inherit
 	// from the cookie).
-	newSess.Options = cookieOptions(!s.config.IsDev())
+	newSess.Options = &sessions.Options{
-	newSess.Options.MaxAge = secondsPerDay * sessionMaxAgeDays
+		Path:     "/",
+		MaxAge:   secondsPerDay * sessionMaxAgeDays,
+		HttpOnly: true,
+		Secure:   !s.config.IsDev(),
+		SameSite: http.SameSiteLaxMode,
+	}
 
 	return newSess, nil
 }

internal/session/session_test.go

@@ -39,19 +39,6 @@ func (c *fakeClock) Advance(d time.Duration) {
 	c.t = c.t.Add(d)
 }
 
-// testKey returns the fixed session key the tests sign with. The
-// codec tests re-sign cookies with it, so it must be the same key the
-// store was built from.
-func testKey() []byte {
-	key := make([]byte, testKeySize)
-
-	for i := range key {
-		key[i] = byte(i + 42)
-	}
-
-	return key
-}
-
 // testSession creates a Session with a real cookie store and the
 // real clock.
 func testSession(t *testing.T) *session.Session {
@@ -72,8 +59,20 @@ func testSessionWithClock(
 ) (*session.Session, *fakeClock) {
 	t.Helper()
 
-	key := testKey()
+	key := make([]byte, testKeySize)
-	store := session.NewStore(key, false)
+
+	for i := range key {
+		key[i] = byte(i + 42)
+	}
+
+	store := sessions.NewCookieStore(key)
+	store.Options = &sessions.Options{
+		Path:     "/",
+		MaxAge:   86400 * 7,
+		HttpOnly: true,
+		Secure:   false,
+		SameSite: http.SameSiteLaxMode,
+	}
 
 	cfg := &config.Config{
 		Environment:        config.EnvironmentDev,
@@ -646,34 +645,6 @@ func TestTouch_LazyBelowRefreshThreshold(t *testing.T) {
 	)
 }
 
-func TestTouch_RefreshThresholdIsOneTenthOfIdleWindow(t *testing.T) {
-	t.Parallel()
-
-	// testRefreshDivisor restates the documented bound independently
-	// of the implementation constant: the idle timestamp is rewritten
-	// once it is a tenth of the idle window old, which is what makes
-	// "expires up to 10% early, never late" true. Both assertions are
-	// needed to pin it -- a larger divisor fails the first, a smaller
-	// one fails the second.
-	const testRefreshDivisor = 10
-
-	threshold := testIdleTimeout / testRefreshDivisor
-
-	s, sess, clock := authenticatedSession(t, testIdleTimeout)
-
-	clock.Advance(threshold - time.Second)
-	assert.False(
-		t, s.Touch(sess),
-		"Touch must not rewrite the session below a tenth of the window",
-	)
-
-	clock.Advance(time.Second)
-	assert.True(
-		t, s.Touch(sess),
-		"Touch must rewrite the session at a tenth of the window",
-	)
-}
-
 func TestTouch_UnauthenticatedSessionIsNotRefreshed(t *testing.T) {
 	t.Parallel()