Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions core/db.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 5 additions & 2 deletions extension/public/background.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment on lines 117 to +121
if (!d || !d.skipped) lastIngestHash.set(dedupKey, hash);
sendResponse({ ok: true });
})
.catch(() => {
Expand Down
17 changes: 8 additions & 9 deletions routes/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Expand Down
142 changes: 142 additions & 0 deletions tests/test_ingest_project_tasks_race.py
Original file line number Diff line number Diff line change
@@ -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")
Comment on lines +49 to +52

# 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
Comment on lines +69 to +72


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