From 617e6d199f204f12863f5924c54e563ebc942c5b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 16:30:28 +0000 Subject: [PATCH 1/2] Keep the current session per request, not per store A store is shared by every request but held the session in one field. A visitor with no cookie therefore kept the previous request's session and was sent that session's id as a cookie, which hands over the previous user's session. Concurrent requests overwrote each other the same way. Store now keeps the current session per fiber and SessionHandler releases it when the request ends, so each request starts from a fresh session unless its cookie names a stored one. Flash messages and the cookie store's request cookies are still shared state and are not changed here. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01HT5gvYGR83U2M4DyA6NA6f --- spec/request_isolation_spec.cr | 67 +++++++++++++++++++++++++++++ src/handlers/session_handler.cr | 2 + src/store.cr | 20 +++++++-- src/stores/clustered_redis_store.cr | 1 - src/stores/cookie_store.cr | 1 - src/stores/memory_store.cr | 1 - src/stores/redis_store.cr | 1 - 7 files changed, 85 insertions(+), 8 deletions(-) create mode 100644 spec/request_isolation_spec.cr diff --git a/spec/request_isolation_spec.cr b/spec/request_isolation_spec.cr new file mode 100644 index 0000000..bbd37d9 --- /dev/null +++ b/spec/request_isolation_spec.cr @@ -0,0 +1,67 @@ +require "./spec_helper" + +private class Probe + include HTTP::Handler + + getter seen = [] of String + property? yield_mid_request = false + + def call(context : HTTP::Server::Context) + Fiber.yield if yield_mid_request? + seen << Session.config.store.as(Session::MemoryStore(UserSession)).current_session.session_id + end +end + +private def request_with(session_id : String?) : HTTP::Server::Context + request = HTTP::Request.new("GET", "/") + if session_id + request.cookies << HTTP::Cookie.new(name: Session.config.session_key, value: session_id) + end + HTTP::Server::Context.new(request, HTTP::Server::Response.new(IO::Memory.new)) +end + +describe "request isolation" do + it "gives a visitor without a cookie a session of their own, not the previous visitor's" do + reset_config + store = Session::MemoryStore(UserSession).new + Session.config.store = store + owner = UserSession.new + owner.username = "alice" + store[owner.session_id] = owner + + handler = Session::SessionHandler(UserSession).new(store) + handler.next = Probe.new + + handler.call(request_with(owner.session_id)) + anonymous = request_with(nil) + handler.call(anonymous) + + anonymous.response.cookies[Session.config.session_key]?.try(&.value).should_not eq owner.session_id + end + + it "keeps concurrent requests on their own sessions" do + reset_config + store = Session::MemoryStore(UserSession).new + Session.config.store = store + alice = UserSession.new + bob = UserSession.new + store[alice.session_id] = alice + store[bob.session_id] = bob + + probe = Probe.new + probe.yield_mid_request = true + handler = Session::SessionHandler(UserSession).new(store) + handler.next = probe + + done = Channel(Nil).new + [alice, bob].each do |owner| + spawn do + handler.call(request_with(owner.session_id)) + done.send(nil) + end + end + 2.times { done.receive } + + probe.seen.sort.should eq [alice.session_id, bob.session_id].sort + end +end diff --git a/src/handlers/session_handler.cr b/src/handlers/session_handler.cr index 59675e0..cd00555 100644 --- a/src/handlers/session_handler.cr +++ b/src/handlers/session_handler.cr @@ -9,6 +9,8 @@ module Session load_session(context) call_next(context) save_session(context) + ensure + @session.release end private def load_session(context : HTTP::Server::Context) diff --git a/src/store.cr b/src/store.cr index 7faea02..7960b69 100644 --- a/src/store.cr +++ b/src/store.cr @@ -22,10 +22,21 @@ module Session # Storage abstraction - must be implemented by subclasses abstract def storage : String - # Current session must be provided by subclasses - # Typically implemented as: property current_session : T = T.new - abstract def current_session : T - abstract def current_session=(current_session : T) + # The session of the request being served. A store is shared by every request, so it is + # kept per fiber: concurrent requests, and the next request on a keep-alive connection, + # never see each other's session. `SessionHandler` calls `release` when a request ends. + def current_session : T + @mutex.synchronize { @current_sessions[Fiber.current] ||= T.new } + end + + def current_session=(current_session : T) + @mutex.synchronize { @current_sessions[Fiber.current] = current_session } + end + + # Forget the current fiber's session once its request is done. + def release : Nil + @mutex.synchronize { @current_sessions.delete(Fiber.current) } + end # Core store operations abstract def [](key : String) : T @@ -37,6 +48,7 @@ module Session # Instance variables for session lifecycle @mutex : Mutex = Mutex.new + @current_sessions = {} of Fiber => T @flash : Flash = Flash.new # Access flash messages diff --git a/src/stores/clustered_redis_store.cr b/src/stores/clustered_redis_store.cr index 70ef5d5..2b0278e 100644 --- a/src/stores/clustered_redis_store.cr +++ b/src/stores/clustered_redis_store.cr @@ -26,7 +26,6 @@ module Session getter redis_store : RedisStore(T) getter coordinator : ClusterCoordinator(T) getter circuit_breaker : CircuitBreaker? - property current_session : T = T.new def initialize(@client : Redis = Redis.new, config : ClusterConfig = ClusterConfig.new) @redis_store = RedisStore(T).new(@client) diff --git a/src/stores/cookie_store.cr b/src/stores/cookie_store.cr index ffae345..7ca56f9 100644 --- a/src/stores/cookie_store.cr +++ b/src/stores/cookie_store.cr @@ -3,7 +3,6 @@ require "../store" module Session class CookieStore(T) < Store(T) include Enumerable(HTTP::Cookie) - property current_session : T = T.new property cookies : HTTP::Cookies = HTTP::Cookies.new def initialize(@cookies : HTTP::Cookies = HTTP::Cookies.new) diff --git a/src/stores/memory_store.cr b/src/stores/memory_store.cr index c393644..19c9749 100644 --- a/src/stores/memory_store.cr +++ b/src/stores/memory_store.cr @@ -3,7 +3,6 @@ module Session include Enumerable(T) include QueryableStore(T) getter sessions : Hash(String, T) - property current_session : T = T.new def initialize(@sessions : Hash(String, T) = Hash(String, T).new) end diff --git a/src/stores/redis_store.cr b/src/stores/redis_store.cr index 0e3d0a4..029d1fc 100644 --- a/src/stores/redis_store.cr +++ b/src/stores/redis_store.cr @@ -5,7 +5,6 @@ module Session class RedisStore(T) < Store(T) include QueryableStore(T) getter circuit_breaker : CircuitBreaker? - property current_session : T = T.new @client : Redis? @pool : ConnectionPool? From 82f4b14b71260fa87ca35c579bc293dbfb678fd6 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 16:36:41 +0000 Subject: [PATCH 2/2] Prune finished fibers from the per-request session table Review found that the getter inserted a session for any fiber that read it and only SessionHandler released, so fibers that read the session outside a request (a spawned task, a WebSocket channel) were kept for good. Inserting now drops entries of fibers that have finished. A fiber spawned during a request still starts with a blank session; the store documents that it must capture the session before spawn. The release spec now materializes the session before raising, so it fails without the ensure, and the cookie spec asserts the cookie exists. Co-Authored-By: Claude Sonnet 5.5 Claude-Session: https://claude.ai/code/session_01HT5gvYGR83U2M4DyA6NA6f --- docs/api/store.md | 5 +++-- spec/request_isolation_spec.cr | 32 +++++++++++++++++++++++++++++++- src/store.cr | 12 ++++++++++-- 3 files changed, 44 insertions(+), 5 deletions(-) diff --git a/docs/api/store.md b/docs/api/store.md index 9877d0e..91d8003 100644 --- a/docs/api/store.md +++ b/docs/api/store.md @@ -9,8 +9,9 @@ Subclasses must implement: | Method | Signature | Description | |--------|-----------|-------------| | `storage` | `: String` | Backend name (e.g., `"memory"`, `"redis"`) | -| `current_session` | `: T` | Get current session | -| `current_session=` | `(T)` | Set current session | +| `current_session` | `: T` | Get the current request's session (kept per fiber) | +| `current_session=` | `(T)` | Set the current request's session | +| `release` | `: Nil` | Forget the current fiber's session; `SessionHandler` calls it when a request ends | | `[]` | `(key : String) : T` | Fetch session by ID (raises `SessionNotFoundException`) | | `[]?` | `(key : String) : T?` | Fetch session by ID (returns `nil`) | | `[]=` | `(key : String, session : T) : T` | Store a session | diff --git a/spec/request_isolation_spec.cr b/spec/request_isolation_spec.cr index bbd37d9..e0e43e1 100644 --- a/spec/request_isolation_spec.cr +++ b/spec/request_isolation_spec.cr @@ -36,7 +36,9 @@ describe "request isolation" do anonymous = request_with(nil) handler.call(anonymous) - anonymous.response.cookies[Session.config.session_key]?.try(&.value).should_not eq owner.session_id + cookie = anonymous.response.cookies[Session.config.session_key]? + cookie.should_not be_nil + cookie.try(&.value).should_not eq owner.session_id end it "keeps concurrent requests on their own sessions" do @@ -64,4 +66,32 @@ describe "request isolation" do probe.seen.sort.should eq [alice.session_id, bob.session_id].sort end + + it "releases the request's session when the request ends, even when it raises" do + reset_config + store = Session::MemoryStore(UserSession).new + Session.config.store = store + + handler = Session::SessionHandler(UserSession).new(store) + handler.next = ->(_context : HTTP::Server::Context) do + store.current_session + raise "boom" + end + + expect_raises(Exception, "boom") { handler.call(request_with(nil)) } + store.@current_sessions.should be_empty + end + + it "does not keep sessions of fibers that have finished" do + reset_config + store = Session::MemoryStore(UserSession).new + + done = Channel(Nil).new + 50.times { spawn { store.current_session; done.send(nil) } } + 50.times { done.receive } + Fiber.yield + + store.current_session + store.@current_sessions.size.should be < 5 + end end diff --git a/src/store.cr b/src/store.cr index 7960b69..617101b 100644 --- a/src/store.cr +++ b/src/store.cr @@ -25,12 +25,20 @@ module Session # The session of the request being served. A store is shared by every request, so it is # kept per fiber: concurrent requests, and the next request on a keep-alive connection, # never see each other's session. `SessionHandler` calls `release` when a request ends. + # + # A fiber spawned during a request starts with a blank session; capture the session + # before `spawn` to use it there. def current_session : T - @mutex.synchronize { @current_sessions[Fiber.current] ||= T.new } + @mutex.synchronize { @current_sessions[Fiber.current]? || remember(T.new) } end def current_session=(current_session : T) - @mutex.synchronize { @current_sessions[Fiber.current] = current_session } + @mutex.synchronize { remember(current_session) } + end + + private def remember(session : T) : T + @current_sessions.reject! { |fiber, _| fiber.dead? } + @current_sessions[Fiber.current] = session end # Forget the current fiber's session once its request is done.