From 03754613fcfed5caeb7f0ca87200908f2769526c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Dlouh=C3=BD?= Date: Fri, 28 Aug 2026 09:11:05 +0200 Subject: [PATCH 1/4] Keep confirm_human from overwriting concurrent session writes confirm_human() writes its session flag and then replays the participant's enrollments and goals - a counter round trip each - while the session is only saved when the response is returned. On a busy site that replay takes seconds, and the save then writes back the whole session dict as it looked when the request loaded it. Anything a concurrent request wrote to the same session in the meantime is silently lost. The way this was found: python-social-auth stores its OAuth `state` in the session on /login//. When a confirm_human ping (fired from every page load for new visitors) overlapped the login, the state was erased and the provider callback failed with AuthStateMissing. Measured on a production-like Heroku app: POST /experiments/confirm_human/ 13.241 service=3998ms -> saved 17.239 POST /login/facebook/ 13.299 service=1618ms -> saved 14.917 GET /complete/facebook/ 18.851 AuthStateMissing The view now re-reads the stored session after confirm_human() has run, writes only the keys that actually changed, and keeps the middleware from saving the stale snapshot. Cookie-backed sessions are left alone: they have no server-side store to race on, and suppressing the middleware save would drop the response cookie that is their persistence. The regression test injects a concurrent write inside confirm_human(), which is exactly where such requests land; it fails on master with the symptom above (the concurrent key reads back as None) and passes with this change. --- experiments/tests/test_views.py | 76 +++++++++++++++++++++++++++++++++ experiments/views.py | 53 +++++++++++++++++++++-- 2 files changed, 126 insertions(+), 3 deletions(-) create mode 100644 experiments/tests/test_views.py diff --git a/experiments/tests/test_views.py b/experiments/tests/test_views.py new file mode 100644 index 00000000..d0744180 --- /dev/null +++ b/experiments/tests/test_views.py @@ -0,0 +1,76 @@ +from __future__ import absolute_import + +from django.contrib.sessions.backends.db import SessionStore as DatabaseSession +from django.test import TestCase +from django.urls import reverse + +from experiments import conf +from experiments.utils import WebUser + +from mock import patch + +OAUTH_STATE_KEY = 'oauth_state' +OAUTH_STATE = 'state-written-by-a-concurrent-request' + + +class ConfirmHumanViewTest(TestCase): + def setUp(self): + session = DatabaseSession() + session['seeded'] = 'before' + session.save() + self.session_key = session.session_key + self.client.cookies['sessionid'] = self.session_key + self.url = reverse('experiment_confirm_human') + + def stored(self): + return DatabaseSession(session_key=self.session_key) + + def test_get_is_rejected(self): + response = self.client.get(self.url) + self.assertEqual(response.status_code, 405) + + def test_post_marks_the_session_as_human(self): + response = self.client.post(self.url) + self.assertEqual(response.status_code, 204) + self.assertTrue(self.stored().get(conf.CONFIRM_HUMAN_SESSION_KEY)) + + def test_leaves_untouched_keys_alone(self): + self.client.post(self.url) + self.assertEqual(self.stored().get('seeded'), 'before') + + def test_does_not_drop_a_concurrent_session_write(self): + """A write landing while confirm_human replays counters must survive. + + confirm_human() can spend seconds replaying enrollments and goals to + the counter store. A concurrent request writing to the same session in + that window (an OAuth login storing its state, for example) used to be + overwritten when this request's stale session snapshot was saved at the + end of the request. The concurrent write is injected inside + confirm_human(), which is exactly where such requests land. + """ + original = WebUser.confirm_human + + def concurrent_write_then_confirm(user): + concurrent = DatabaseSession(session_key=self.session_key) + concurrent[OAUTH_STATE_KEY] = OAUTH_STATE + concurrent.save() + return original(user) + + with patch.object(WebUser, 'confirm_human', concurrent_write_then_confirm): + response = self.client.post(self.url) + + self.assertEqual(response.status_code, 204) + stored = self.stored() + self.assertEqual(stored.get(OAUTH_STATE_KEY), OAUTH_STATE) + self.assertTrue(stored.get(conf.CONFIRM_HUMAN_SESSION_KEY)) + + def test_repeat_ping_makes_no_session_write(self): + self.client.post(self.url) + with patch.object(DatabaseSession, 'save') as save: + self.client.post(self.url) + save.assert_not_called() + + def test_without_a_session_cookie_it_still_answers(self): + del self.client.cookies['sessionid'] + response = self.client.post(self.url) + self.assertEqual(response.status_code, 204) diff --git a/experiments/views.py b/experiments/views.py index c8bdada0..d839d861 100644 --- a/experiments/views.py +++ b/experiments/views.py @@ -1,3 +1,4 @@ +from django.contrib.sessions.backends.signed_cookies import SessionStore as SignedCookiesStore from django.http import HttpResponse, HttpResponseBadRequest from django.views.decorators.cache import never_cache from django.shortcuts import get_object_or_404 @@ -19,15 +20,61 @@ "\x00\x49\x45\x4e\x44\xae\x42\x60\x82\x00") +_MISSING = object() + + @never_cache @require_POST def confirm_human(request): - if conf.CONFIRM_HUMAN: - experiment_user = participant(request) - experiment_user.confirm_human() + if not conf.CONFIRM_HUMAN: + return HttpResponse(status=204) + + session = getattr(request, 'session', None) + before = dict(session.items()) if session is not None else {} + + experiment_user = participant(request) + experiment_user.confirm_human() + + _save_only_confirm_human_changes(session, before) return HttpResponse(status=204) +def _save_only_confirm_human_changes(session, before): + """Persist this request's session changes without dropping concurrent ones. + + confirm_human() replays the participant's enrollments and goals - a counter + round trip each - between writing its session flag and the session being + saved at the end of the request. That can take seconds, and the middleware + then writes back the whole session dict as it looked when this request + loaded it: a concurrent request that wrote to the same session in the + meantime (an OAuth login storing its state, for example) is silently + overwritten by this request's stale snapshot. + + Instead, re-read the stored session, write only the keys confirm_human + changed, and keep the middleware from saving the stale snapshot. + """ + if session is None or not session.session_key: + # No stored session yet, so there is nothing to race with - let the + # middleware create and save the session as it normally would. + return + if isinstance(session, SignedCookiesStore): + # Cookie-backed sessions have no server-side store to race on; + # persistence is the response cookie the middleware writes. + return + + changed = {key: value for key, value in session.items() if before.get(key, _MISSING) != value} + if not changed: + session.modified = False + return + + fresh = type(session)(session_key=session.session_key) + for key, value in changed.items(): + fresh[key] = value + fresh.save() + # The middleware must not write our stale snapshot over the merge above. + session.modified = False + + @never_cache def record_experiment_goal(request, goal_name, cache_buster=None): participant(request).goal(goal_name) From 931100aa9077d06d9dacc951a1dd36a125b8e573 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Dlouh=C3=BD?= Date: Fri, 28 Aug 2026 09:39:29 +0200 Subject: [PATCH 2/4] Keep the merge intact under SESSION_SAVE_EVERY_REQUEST With SESSION_SAVE_EVERY_REQUEST=True the session middleware saves even an unmodified session, so resetting session.modified was not enough: the middleware would write this request's stale snapshot over the merged state, re-introducing the lost-update the previous commit fixed (spotted by CodeRabbit on the PR). Point the request's session object at the merged contents instead - then whatever the middleware decides to persist is the merged state, never the stale snapshot. The regression test drives the same concurrent write under override_settings(SESSION_SAVE_EVERY_REQUEST=True); it fails on the previous commit and passes here. Also adds the docstrings the review tooling flagged. --- experiments/tests/test_views.py | 30 +++++++++++++++++++++++++++++- experiments/views.py | 24 +++++++++++++++++------- 2 files changed, 46 insertions(+), 8 deletions(-) diff --git a/experiments/tests/test_views.py b/experiments/tests/test_views.py index d0744180..fa69526e 100644 --- a/experiments/tests/test_views.py +++ b/experiments/tests/test_views.py @@ -1,7 +1,7 @@ from __future__ import absolute_import from django.contrib.sessions.backends.db import SessionStore as DatabaseSession -from django.test import TestCase +from django.test import TestCase, override_settings from django.urls import reverse from experiments import conf @@ -15,6 +15,7 @@ class ConfirmHumanViewTest(TestCase): def setUp(self): + """Store a session with a seeded key and point the test client at it.""" session = DatabaseSession() session['seeded'] = 'before' session.save() @@ -23,18 +24,22 @@ def setUp(self): self.url = reverse('experiment_confirm_human') def stored(self): + """Re-read the session from the store, bypassing the request copy.""" return DatabaseSession(session_key=self.session_key) def test_get_is_rejected(self): + """The endpoint stays POST-only.""" response = self.client.get(self.url) self.assertEqual(response.status_code, 405) def test_post_marks_the_session_as_human(self): + """The primary effect: the confirmed-human flag lands in the session.""" response = self.client.post(self.url) self.assertEqual(response.status_code, 204) self.assertTrue(self.stored().get(conf.CONFIRM_HUMAN_SESSION_KEY)) def test_leaves_untouched_keys_alone(self): + """Keys confirm_human never touched keep their stored values.""" self.client.post(self.url) self.assertEqual(self.stored().get('seeded'), 'before') @@ -65,12 +70,35 @@ def concurrent_write_then_confirm(user): self.assertTrue(stored.get(conf.CONFIRM_HUMAN_SESSION_KEY)) def test_repeat_ping_makes_no_session_write(self): + """A ping that changes nothing must not write the session at all.""" self.client.post(self.url) with patch.object(DatabaseSession, 'save') as save: self.client.post(self.url) save.assert_not_called() def test_without_a_session_cookie_it_still_answers(self): + """A cookieless request (bot, first hit) is answered, not crashed.""" del self.client.cookies['sessionid'] response = self.client.post(self.url) self.assertEqual(response.status_code, 204) + + def test_concurrent_write_survives_save_every_request(self): + """SESSION_SAVE_EVERY_REQUEST makes the middleware save even an + unmodified session, so the merged state must also be what the request's + own session object holds by the time the middleware runs.""" + original = WebUser.confirm_human + + def concurrent_write_then_confirm(user): + concurrent = DatabaseSession(session_key=self.session_key) + concurrent[OAUTH_STATE_KEY] = OAUTH_STATE + concurrent.save() + return original(user) + + with override_settings(SESSION_SAVE_EVERY_REQUEST=True): + with patch.object(WebUser, 'confirm_human', concurrent_write_then_confirm): + response = self.client.post(self.url) + + self.assertEqual(response.status_code, 204) + stored = self.stored() + self.assertEqual(stored.get(OAUTH_STATE_KEY), OAUTH_STATE) + self.assertTrue(stored.get(conf.CONFIRM_HUMAN_SESSION_KEY)) diff --git a/experiments/views.py b/experiments/views.py index d839d861..5df16461 100644 --- a/experiments/views.py +++ b/experiments/views.py @@ -26,6 +26,11 @@ @never_cache @require_POST def confirm_human(request): + """Mark the session as belonging to a human, without clobbering the session. + + See _save_only_confirm_human_changes for why the session handling is not + left to the middleware. + """ if not conf.CONFIRM_HUMAN: return HttpResponse(status=204) @@ -63,15 +68,20 @@ def _save_only_confirm_human_changes(session, before): return changed = {key: value for key, value in session.items() if before.get(key, _MISSING) != value} - if not changed: - session.modified = False - return fresh = type(session)(session_key=session.session_key) - for key, value in changed.items(): - fresh[key] = value - fresh.save() - # The middleware must not write our stale snapshot over the merge above. + if changed: + for key, value in changed.items(): + fresh[key] = value + fresh.save() + + # The middleware must not write this request's stale snapshot over the + # merge above. With SESSION_SAVE_EVERY_REQUEST=True it saves even an + # unmodified session, so the request's session object is also pointed at + # the merged state - whatever the middleware does, it persists that. + # (_session_cache is the only way to replace the contents without + # marking the session dirty.) + session._session_cache = dict(fresh.items()) session.modified = False From c09d9afe8bdd4e68fb9659f9ee23498642e8ec35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Dlouh=C3=BD?= Date: Fri, 28 Aug 2026 09:39:52 +0200 Subject: [PATCH 3/4] Repair the CI matrix: map Python 3.12 in tox, drop unavailable 3.7 Two time bombs, both visible on any PR today: * The workflow has run Python 3.12 since the Django 5.0 update, but [gh-actions] in tox.ini never learned the mapping - so on 3.12 tox-gh-actions falls back to the bare 'py' env, which has no Django pin. That installed whatever was newest; since Django 6 released, 'makemigrations --check' demands a BigAutoField migration and the job fails before a single test runs. Mapping 3.12 to the py312 envs runs the pinned Django 4.2/5.0 matrix instead. * Python 3.7 is no longer available on the ubuntu-latest runner images ('Version 3.7 with arch x64 not found'), so that job cannot even set up. Dropped from the workflow; the py37 tox envs remain for anyone running tox locally on an interpreter that has it. --- .github/workflows/tests.yml | 4 +++- tox.ini | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 98d576bb..ca433e4e 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -12,7 +12,9 @@ jobs: strategy: max-parallel: 5 matrix: - python-version: ['3.7', '3.8', '3.9', '3.10', '3.11', '3.12', 'pypy-3.8', 'pypy-3.9', 'pypy-3.10'] + # 3.7 is gone from the ubuntu-latest runner images (setup-python: + # "Version 3.7 with arch x64 not found"). + python-version: ['3.8', '3.9', '3.10', '3.11', '3.12', 'pypy-3.8', 'pypy-3.9', 'pypy-3.10'] fail-fast: false steps: diff --git a/tox.ini b/tox.ini index 5a07e4bf..b0e4d230 100644 --- a/tox.ini +++ b/tox.ini @@ -27,6 +27,7 @@ python = 3.9: py39 3.10: py310 3.11: py311 + 3.12: py312 pypy-3.8: pypy38 pypy-3.9: pypy39 pypy-3.10: pypy310 From 6e93e9ed6f4b77bda1d429d663cfb2b3d5a5ab0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20Dlouh=C3=BD?= Date: Fri, 28 Aug 2026 09:52:25 +0200 Subject: [PATCH 4/4] Support current Django (5.1-6.1) and extend the test matrix Two real incompatibilities surfaced once the resolver was allowed to install modern Django: * Django 6.1 gave SessionBase a __bool__, so an *empty* session is now falsy. Two truthiness checks changed meaning: _get_participant demoted every fresh visitor to a DummyUser (no enrollment, nothing counted), and _session_key returned None for them, keying every such visitor's enrollments and counters to the same identifier - the test suite's MultipleObjectsReturned came from exactly that collision. Both are now identity checks. * Django >= 6 defaults DEFAULT_AUTO_FIELD to BigAutoField, which made makemigrations demand an id migration from every project. The app now pins its historical AutoField in the AppConfig, so existing installations are not asked to alter their tables. The tox envlist grows django5.1/5.2 (py310-313) and django6.0/6.1 (py312-313), the workflow matrix gains Python 3.13, and [gh-actions] learns the 3.13 mapping. Verified locally on Django 5.0.14, 5.1.15, 5.2.17, 6.0.8 and 6.1: makemigrations --check clean and the full suite OK on each. --- .github/workflows/tests.yml | 2 +- experiments/apps.py | 3 +++ experiments/utils.py | 12 +++++++++--- tox.ini | 7 +++++++ 4 files changed, 20 insertions(+), 4 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index ca433e4e..c861deea 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -14,7 +14,7 @@ jobs: matrix: # 3.7 is gone from the ubuntu-latest runner images (setup-python: # "Version 3.7 with arch x64 not found"). - python-version: ['3.8', '3.9', '3.10', '3.11', '3.12', 'pypy-3.8', 'pypy-3.9', 'pypy-3.10'] + python-version: ['3.8', '3.9', '3.10', '3.11', '3.12', '3.13', 'pypy-3.8', 'pypy-3.9', 'pypy-3.10'] fail-fast: false steps: diff --git a/experiments/apps.py b/experiments/apps.py index e7515bb0..b45a0cc3 100644 --- a/experiments/apps.py +++ b/experiments/apps.py @@ -4,6 +4,9 @@ class ExperimentsConfig(AppConfig): name = 'experiments' label = 'experiments' + # Django >= 6 defaults new projects to BigAutoField; pin the historical + # AutoField so existing installations are not asked for an id migration. + default_auto_field = 'django.db.models.AutoField' def ready(self): from django.contrib.auth.signals import user_logged_in, user_logged_out diff --git a/experiments/utils.py b/experiments/utils.py index 52b9a2dd..c3bf567e 100644 --- a/experiments/utils.py +++ b/experiments/utils.py @@ -54,7 +54,7 @@ def clear_participant_cache(request): def _get_participant(request, session, user): if request and hasattr(request, 'user') and not user: user = request.user - if request and hasattr(request, 'session') and not session: + if request and hasattr(request, 'session') and session is None: session = request.session if request and conf.BOT_REGEX.search(request.META.get("HTTP_USER_AGENT", "")): @@ -64,7 +64,10 @@ def _get_participant(request, session, user): return WebUser(user=user, request=request) else: return DummyUser() - elif session: + elif session is not None: + # Truthiness is not identity here: since Django 6.1 an *empty* session + # is falsy, and a fresh visitor's empty session must still get a + # WebUser, not a DummyUser. return WebUser(session=session, request=request) else: return DummyUser() @@ -409,7 +412,10 @@ def _is_verified_human(self): @property def _session_key(self): - if not self.session: + # `is None`, not truthiness: since Django 6.1 an *empty* session is + # falsy, and returning None for every fresh visitor would key all of + # their enrollments and counters to the same identifier. + if self.session is None: return None if 'experiments_session_key' not in self.session: if not self.session.session_key: diff --git a/tox.ini b/tox.ini index b0e4d230..d661ed23 100644 --- a/tox.ini +++ b/tox.ini @@ -17,6 +17,8 @@ envlist = {py,pypy}{38,39,310,311}-django{4.1} {py,pypy}{38,39,310,311,312}-django{4.2} {py,pypy}{310,311,312}-django{5.0} + {py,pypy}{310,311,312,313}-django{5.1,5.2} + py{312,313}-django{6.0,6.1} [gh-actions] python = @@ -28,6 +30,7 @@ python = 3.10: py310 3.11: py311 3.12: py312 + 3.13: py313 pypy-3.8: pypy38 pypy-3.9: pypy39 pypy-3.10: pypy310 @@ -52,3 +55,7 @@ deps = django4.1: Django==4.1.* django4.2: Django==4.2.* django5.0: Django==5.0.* + django5.1: Django==5.1.* + django5.2: Django==5.2.* + django6.0: Django==6.0.* + django6.1: Django==6.1.*