Skip to content

Clean up expired sessions in SessionMiddleware #3

Closed

Description

@tompscanlan

Summary

When the SessionMiddleware detects an expired session, it continues without the user but doesn't clean up the expired session from the database. This leads to stale session data accumulating over time.

Before You Start

⚠️ Verification Required: Please confirm this issue still exists before starting work:

  1. Open internal/oauth/middleware.go
  2. Find the TODO comment around line 42
  3. Verify the expired session is not being deleted
  4. Comment below confirming the issue exists

Current Code

// Lines 39-44 in middleware.go
// Check if session is expired
if session.ExpiresAt.Before(time.Now()) {
    // Expired session - continue without user
    // TODO: Consider cleaning up expired session here
    return next(c)
}

Expected Fix

Delete the expired session before continuing:

// Check if session is expired
if session.ExpiresAt.Before(time.Now()) {
    // Clean up expired session
    if err := storage.DeleteSession(c.Request().Context(), cookie.Value); err != nil {
        c.Logger().Errorf("Failed to delete expired session: %v", err)
    }
    return next(c)
}

Why This Matters

  • Expired sessions accumulate in the database indefinitely
  • Database storage grows unnecessarily
  • Could impact query performance over time

Acceptance Criteria

  • Call storage.DeleteSession() when expired session is detected
  • Log any errors from deletion (don't fail the request)
  • Add a unit test verifying cleanup is called for expired sessions
  • Run tests: go test ./internal/oauth/...

Helpful Context

  • The storage parameter already has DeleteSession method (see storage.go:191)
  • Similar cleanup patterns exist in handlers.go (lines 896, 1206, 1299)
  • Error from deletion should be logged but not block the request

Estimated Time

30-45 minutes

Activity

  1. moved this from Backlog to Ready in OpenMeet Work Boardon Dec 26, 2025
  2. moved this from Ready to In progress in OpenMeet Work Boardon Jan 3, 2026
  3. tompscanlan commented on Jan 3, 2026

    @tompscanlan
    ContributorAuthor

    @majiayu000 I see you made a commit for this in your own repo. If you make a pull request, I can review it and work with you to get it ready to merge into the main repo.

  4. majiayu000 commented on Jan 4, 2026

    @majiayu000
    Contributor

    Verified the issue exists and PR #7 now addresses it: SessionMiddleware deletes expired sessions and logs errors without blocking. I also added a non-e2e unit test to cover expired-session cleanup, delete-error non-blocking, and valid-session path; pushed to the PR. CI (test job) is green.

  5. moved this from In progress to Done in OpenMeet Work Boardon Jan 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workinggood first issueGood for newcomers

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions