Skip to content

Session registry has no cap, and every maintenance tick rescans it under jw.mu #396

Description

@linkdata

Found during a source audit at 806bafb. Two halves of the same problem: nothing bounds the Session registry, and every maintenance tick walks all of it while holding the process-global write lock.

What happens

sessioner.ServeHTTP (session.go:512) mints a Session for every request whose cookie does not resolve, before the wrapped handler runs. There is no per-IP cap and no global cap — newSessionLocked's only rejection is post-Close. Every Session stays registered for at least the one-minute deadline set at session.go:39.

maintenance (serve.go:198-217) then takes jw.mu.Lock() and, inside that one hold, ranges the whole Request registry, retires every expired Request inline, and ranges the whole Session registry taking each Session.mu via isDead(). jw.mu is the lock every JaWS HTTP entry point needs, and the pass runs on the single Serve goroutine, so it is a stop-the-world for the library at 1 Hz.

Registered Sessions therefore track request_rate × 60 s, and that is exactly what each pass walks.

Measured

Over real HTTP, all defaults:

  • One source address minted 100,000 Sessions in 3.45 s (29,003/s), while the same address's page GETs were correctly capped at Pending()=100.
  • A 60 s flood of 1,545,042 cookie-less GETs left 1,545,042 Sessions and 1,132 MiB resident.
  • After the flood stopped, an unrelated client's ordinary page load blocked 223.7–296.1 ms in each of the next ten seconds (2.565 s out of ~10 s) while its median stayed at 73 µs. It self-heals about 60 s later.
  • Isolated pass cost: 5.57 ms at 49,992 Sessions, 24.07 ms at 199,992, 75.43 ms at 499,992 (111–150 ns/entry).
  • Retained cost 413.6–415.3 bytes per Session, linear.

The Request half is much smaller than it first looks: the default per-IP cap held exactly, and whole-second lastWriteSeconds bucketing means one pass retires only one runtime-second of arrivals. A 60,000-Request cohort from 600 real source addresses split across two passes for a 45.7 ms worst case.

Simplest fix

The Session scan is the dominant term and the only uncapped one. A Session cannot die before its 60 s deadline, so scanning all of them every second is ~60× more often than necessary. Sweep them every Nth pass:

jw.sessionSweep++
if jw.sessionSweep%10 == 0 {
	for _, sess := range jw.sessions {
		if sess.isDead() { jw.deleteSessionIfCurrentLocked(sess) }
	}
}

Two lines, cuts the session-scan lock duty ~10× and stretches retention from ~60–61 s to ~60–70 s. Session.Close already unregisters directly via deleteSessionIfCurrent (session.go:219), so nothing depends on the sweep for prompt removal — it only reaps deadline expiry.

If the Request half also needs bounding, collect expiry candidates under the lock, release it, then retire in small batches. retireNonRunningRequestCoreLocked already re-checks registry identity and running state, and rq.maintenance has cancelled the context before reporting expiry, so a Request cannot be claimed between phases.

Do not add a default-on MaxSessionsPerIP that evicts. Unregistering a Session discards state its owner can still reach with a valid cookie (AI.md:282-283 promises exactly that), and equalIP collapses every client behind a loopback reverse proxy into one bucket unless TrustForwardedHeaders is set — so a default-on per-IP cap would let this same flood destroy every legitimate user's session. If you want a hard memory ceiling, an off-by-default global MaxSessions that refuses rather than evicts is the safe shape; both existing callers of newSessionLocked already handle a nil return.

Also worth knowing: broadcast.go:160-191 distributeDirt has the same O(registry)-under-jw.mu shape at the 100 ms update tick. Much smaller per entry (nil test plus a pointer append, no per-entry mutex, gated on len(jw.dirty) > 0) and it scans the per-IP-capped Request registry, so it is not part of the measurement above — but a fix applied only to maintenance leaves that shape at 10× the frequency.

Activity

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

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions