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}"