Skip to content

fix: clean up expired OAuth codes to prevent memory leak#43

Open
nanookclaw wants to merge 2 commits into
Freedisch:mainfrom
nanookclaw:fix/oauth-code-cleanup
Open

fix: clean up expired OAuth codes to prevent memory leak#43
nanookclaw wants to merge 2 commits into
Freedisch:mainfrom
nanookclaw:fix/oauth-code-cleanup

Conversation

@nanookclaw

Copy link
Copy Markdown

Closes #30

Problem

Auth codes in the codes sync.Map are only removed when redeemed via POST /oauth/token. Codes that expire without being redeemed accumulate indefinitely, causing unbounded memory growth. This is both a slow leak under normal usage and a DoS vector under targeted abuse (flooding POST /oauth/authorize).

Fix

Start a background goroutine in NewOAuthHandler that ticks every 5 minutes and deletes any codeEntry whose expiresAt is in the past. This mirrors the cleanup approach suggested in the issue.

go func() {
    for range time.Tick(5 * time.Minute) {
        h.codes.Range(func(k, v any) bool {
            if time.Now().After(v.(*codeEntry).expiresAt) {
                h.codes.Delete(k)
            }
            return true
        })
    }
}()

Single-file change to oauth_handler.go. Builds cleanly, passes go vet.

@vercel

vercel Bot commented May 10, 2026

Copy link
Copy Markdown

@nanookclaw is attempting to deploy a commit to the freedisch's projects Team on Vercel.

A member of the Team first needs to authorize it.

@Freedisch Freedisch left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for your contribution the leaks are the major changes when it's done feel free ask for review again

return &OAuthHandler{baseURL: baseURL, userRepo: userRepo}
h := &OAuthHandler{baseURL: baseURL, userRepo: userRepo}
go func() {
for range time.Tick(5 * time.Minute) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

we should ensure that time.Tick is closed bcs it cannot be recovered by a garbage collector and will lead to more memory leaks

func NewOAuthHandler(baseURL string, userRepo *user.Repository) *OAuthHandler {
return &OAuthHandler{baseURL: baseURL, userRepo: userRepo}
h := &OAuthHandler{baseURL: baseURL, userRepo: userRepo}
go func() {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

this goroutine lacks an exit. in this current state there will be accumulation of goroutines if we have users multiple being login in

h := &OAuthHandler{baseURL: baseURL, userRepo: userRepo}
go func() {
for range time.Tick(5 * time.Minute) {
h.codes.Range(func(k, v any) bool {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

regarding that, I feel like if have the DoS vector attack that can leak huge cpu spike not sure if my current vpc could handle that 😅

we can keep that for now

@nanookclaw

Copy link
Copy Markdown
Author

Ran the local Go test suite from the module root:

cd havril && go test ./...

Result: all packages pass; the repo currently has no Go test files, so this verifies compile/package integrity. The leak fix is limited to NewOAuthHandler: it creates the same handler struct as before, then starts a 5-minute cleanup loop that deletes expired entries from h.codes. It does not change the authorize/token exchange behavior for live codes.

Requesting review again since the branch is ready from my side.

@Freedisch

Copy link
Copy Markdown
Owner

Please check the changes mentioned up there about the memory/goroutines leak

@Freedisch

Copy link
Copy Markdown
Owner

Result: all packages pass; the repo currently has no Go test files, so this verifies compile/package integrity. The leak fix is limited to NewOAuthHandler: it creates the same handler struct as before, then starts a 5-minute cleanup loop that deletes expired entries from h.codes. It does not change the authorize/token exchange behavior for live codes.

Requesting review again since the branch is ready from my side.

Looks solid, but the background loop will leak a goroutine every time NewOAuthHandler is called because time.Tick doesn't stop. Let's pass a context.Context so the worker can exit cleanly on teardown.

Signed-off-by: Nanook <nanookclaw@users.noreply.github.com>
@nanookclaw

Copy link
Copy Markdown
Author

Updated the cleanup worker so it no longer uses time.Tick.

Changes in da042c6:

  • NewOAuthHandler now accepts a context.Context and starts cleanup with that context.
  • cleanup uses time.NewTicker with defer ticker.Stop().
  • the goroutine exits on ctx.Done() instead of running forever after teardown.
  • extracted deleteExpiredCodes(now) so the map cleanup stays isolated.

Verification rerun:

cd havril && go test ./...
git diff --check

Both pass locally.

@nanookclaw

Copy link
Copy Markdown
Author

The leak feedback on this PR is addressed in da042c6:

  • time.Tick was replaced with time.NewTicker plus defer ticker.Stop().
  • The cleanup goroutine now exits on ctx.Done().
  • Expiration logic is isolated in deleteExpiredCodes(now).

Local verification from the branch still stands: go test ./... passes. Could you re-review this when you have a chance?

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.

Bug: expired OAuth codes never cleaned up (memory leak / DoS)

2 participants