From c670a24377ee1e7304f33808e3bf74d016cd87b6 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 6 Jul 2026 03:59:06 +0000 Subject: [PATCH] fix: stop dropping current-trimester tasks on the ingest ordering race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit project_tasks and projects land as independent, concurrent /ingest requests from the extension. The project_tasks handler rejected any project_id not yet present in `projects` as an ended unit, but a returning student's current-trimester project_tasks push routinely beat its own projects ingest to the DB — misclassifying it as inactive and silently dropping the tasks (compounded by the extension's ingest dedup caching the "skipped" response as if it had been stored, so it never retried). Remove the eager active_ids rejection — the existing prune step already cleans up stale-trimester data once its own projects ingest lands, so the check isn't needed for correctness and only introduced the race. Also harden the extension's dedup cache to never treat a skipped/ non-stored response as sent, as defense in depth. --- core/db.py | 11 -- extension/public/background.js | 7 +- routes/main.py | 17 ++- tests/test_ingest_project_tasks_race.py | 142 ++++++++++++++++++++++++ 4 files changed, 155 insertions(+), 22 deletions(-) create mode 100644 tests/test_ingest_project_tasks_race.py diff --git a/core/db.py b/core/db.py index 9130ae6..935cedc 100644 --- a/core/db.py +++ b/core/db.py @@ -731,17 +731,6 @@ def get_unit_code(user_id: int, project_id: int) -> str | None: return row[0] if row else None -def get_active_project_ids(user_id: int) -> set[int]: - """Project IDs currently stored for the user. The projects ingest only keeps - non-ended units (and prune_ended_projects drops the rest), so this is the set - of *active* projects — used to reject task pushes for past-trimester projects - that an older extension build would otherwise still sweep.""" - with _connection() as conn: - cur = conn.cursor() - cur.execute(f"SELECT project_id FROM projects WHERE user_id = {_P}", (user_id,)) - return {row[0] for row in cur.fetchall()} - - def delete_tasks_for_inactive_projects(user_id: int) -> int: """Delete task rows whose project is no longer in the user's active projects (e.g. past-trimester tasks left behind after prune_ended_projects). Returns the diff --git a/extension/public/background.js b/extension/public/background.js index c0c5b74..d8d3e59 100644 --- a/extension/public/background.js +++ b/extension/public/background.js @@ -115,8 +115,11 @@ chrome.runtime.onMessage.addListener((msg, _sender, sendResponse) => { }), }) .then((r) => r.json()) - .then(() => { - lastIngestHash.set(dedupKey, hash); // only cache once server has accepted it + .then((d) => { + // Only cache once the server actually stored it — a `skipped` response + // (e.g. rejected as an inactive project) must not be treated as sent, or + // this dedup would permanently suppress a push that never landed. + if (!d || !d.skipped) lastIngestHash.set(dedupKey, hash); sendResponse({ ok: true }); }) .catch(() => { diff --git a/routes/main.py b/routes/main.py index 403d77f..afd52ad 100644 --- a/routes/main.py +++ b/routes/main.py @@ -11,7 +11,6 @@ from core.clerk_auth import require_clerk_auth from core.db import ( delete_tasks_for_inactive_projects, - get_active_project_ids, get_capture_meta, get_feedback_entries, get_pending_tasks, @@ -291,14 +290,14 @@ def ingest(): project_id = payload.get("project_id") if project_id is None: return {"ok": False, "error": "missing project_id"}, 400 - # Reject pushes for past-trimester projects. The projects ingest only keeps - # active (non-ended) units, so once the user has any projects stored, a - # project_id absent from that set is an ended unit an older extension build - # is still sweeping — store nothing. The `active_ids` empty case allows the - # race where project_tasks arrives before the projects list has landed. - active_ids = get_active_project_ids(user_id) - if active_ids and project_id not in active_ids: - return {"ok": True, "stored": 0, "skipped": "inactive_project"} + # Past-trimester pushes (an older extension build still sweeping an ended + # unit) are cleaned up by prune_ended_projects/delete_tasks_for_inactive_projects + # on the next "projects" ingest, not rejected here. An eager active_ids + # check used to reject pushes for a project not yet present in `projects`, + # but project_tasks and projects land as independent concurrent requests — + # a returning student's current-trimester project_tasks push routinely beat + # its own projects ingest to the DB, got misclassified as "inactive", and + # (compounded by the extension's ingest dedup) never got stored at all. # Guard: enrich_tasks and append_missing_tasks both use # t["task_definition_id"] (hard key access). Drop any task dicts the # extension sent without that field before passing them in. diff --git a/tests/test_ingest_project_tasks_race.py b/tests/test_ingest_project_tasks_race.py new file mode 100644 index 0000000..2adafbe --- /dev/null +++ b/tests/test_ingest_project_tasks_race.py @@ -0,0 +1,142 @@ +""" +Regression test for the project_tasks/projects ingest ordering race. + +A returning student's current-trimester "project_tasks" push can reach +/ingest before that trimester's "projects" push has been stored (they are +independent, concurrent requests from the extension). The project_tasks +handler used to reject any project_id absent from the `projects` table as +"inactive", silently dropping a currently-enrolled unit's tasks. This test +pins the fix: a project_tasks push for a project not yet in `projects` +must be stored, not skipped. +""" + +import os +import sys +import tempfile +import types +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +PROJECT_ROOT = Path(__file__).parent.parent +sys.path.insert(0, str(PROJECT_ROOT)) + +# Stub out heavyweight/external-service imports the same way +# test_refresh_token_expiry.py does, so importing app/routes doesn't need +# real Postgres/Resend/Clerk/Sentry configuration. +ext_mod = types.ModuleType("extensions") +ext_mod.scheduler = MagicMock() +ext_mod.limiter = MagicMock() +ext_mod.limiter.limit.return_value = lambda f: f +sys.modules["extensions"] = ext_mod + +sys.modules.setdefault("resend", MagicMock()) +sys.modules.setdefault("jwt", MagicMock()) + +crypto_mod = types.ModuleType("core.crypto") +crypto_mod.encrypt = lambda x: f"ENC:{x}" +crypto_mod.decrypt = lambda x: x[4:] if x and x.startswith("ENC:") else (x or "") +sys.modules["core.crypto"] = crypto_mod + + +@pytest.fixture() +def client(monkeypatch): + """A Flask test client backed by a fresh, throwaway SQLite DB per test.""" + db_fd, db_path = tempfile.mkstemp(suffix=".db") + os.close(db_fd) + os.remove(db_path) # let init_db() create it fresh + monkeypatch.setenv("DB_PATH", db_path) + monkeypatch.setenv("SECRET_KEY", "test-secret") + monkeypatch.setenv("RESEND_API_KEY", "test-key") + monkeypatch.setenv("RESEND_FROM_EMAIL", "test@example.com") + + # core.db reads DB_PATH at import time, so import it only now. + for mod in ("core.db", "app", "routes.main"): + sys.modules.pop(mod, None) + import core.db as db + + db.init_db() + db.upsert_user( + base_url="https://ontrack.deakin.edu.au", + username="s1234567", + auth_token="tok", + email="student@test.com", + ) + + from app import create_app + + flask_app = create_app() + flask_app.config["TESTING"] = True + with flask_app.test_client() as c: + yield c + + +def test_project_tasks_stored_when_projects_row_not_yet_landed(client): + """The race case: project_tasks for a unit arrives before that unit's row + has been upserted into `projects` at all (a brand-new/first-ever ingest). + This must be stored, not skipped.""" + resp = client.post( + "/ingest", + json={ + "username": "s1234567", + "kind": "project_tasks", + "payload": { + "project_id": 555, + "unit_code": "SIT999", + "tasks": [ + { + "task_definition_id": 1, + "status": "not_started", + "task_definition": {"id": 1, "name": "Task 1", "target_grade": 3}, + } + ], + "task_definitions": [{"id": 1, "name": "Task 1", "target_grade": 3}], + }, + }, + ) + data = resp.get_json() + assert resp.status_code == 200, data + assert data.get("skipped") is None, f"push was skipped, should have been stored: {data}" + assert data.get("stored", 0) >= 1, f"expected at least one task stored: {data}" + + +def test_project_tasks_stored_when_stale_projects_from_other_unit_exist(client): + """The real-world race: an OLD trimester's project is still in `projects` + (its own ingest hasn't pruned it yet) when the NEW trimester's project_tasks + push lands first. The new project_id is absent from the old active set — + this must not be treated as "inactive".""" + import core.db as db + + user = db.get_user_by_username("s1234567") + old_project = { + "project_id": 111, + "unit_code": "SIT100", + "unit_name": "Old Unit", + "unit_end_date": "2020-01-01", + } + db.upsert_projects(user["id"], [old_project]) + + resp = client.post( + "/ingest", + json={ + "username": "s1234567", + "kind": "project_tasks", + "payload": { + "project_id": 222, # the new unit — not yet in `projects` + "unit_code": "SIT200", + "tasks": [ + { + "task_definition_id": 9, + "status": "not_started", + "task_definition": {"id": 9, "name": "Task 9", "target_grade": 3}, + } + ], + "task_definitions": [{"id": 9, "name": "Task 9", "target_grade": 3}], + }, + }, + ) + data = resp.get_json() + assert resp.status_code == 200, data + assert data.get("skipped") is None, f"current unit's push was skipped: {data}" + assert data.get("stored", 0) >= 1, f"expected the new unit's task to be stored: {data}"