From 3a7a1c7c3669df729c002b20bbaa39587f62fb67 Mon Sep 17 00:00:00 2001 From: nicowre Date: Tue, 30 Jun 2026 09:58:21 +0200 Subject: [PATCH] fix(deployments): garbage-collect orphan student users on deployment delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nothing in the codebase ever deletes user / course_member / group_member rows — UserSyncService.deactivate_user is a comment-only no-op and isn't called from anywhere. Combined with _sync_student_memberships, which creates one User per student named in every wizard payload, those tables only grow. delete_deployment now does a final cleanup pass after the deployment row is removed: 1. Snapshot all student user_ids tied to the deployment BEFORE the access rows are deleted (we lose the chain afterwards). 2. _gc_orphan_student_memberships walks each user: - drops GroupMember rows whose course_group no longer has a live DeploymentInstanceAccess on any deployment - drops the CourseMember once its last GroupMember is gone - drops the User row once its last CourseMember is gone 3. Safety net: users that own templates or openstack projects are skipped — those are lecturers/admins, never students. This keeps the users/course_members/group_members tables bounded by the number of active deployments instead of growing monotonically. A student in two deployments stays until the second one is deleted; the cleanup re-queries the surviving access rows at each call so it's safe to chain. Tests cover: orphan student is dropped end-to-end; user with active access on another deployment is kept; template-owner / openstack-owner safety nets fire; empty input set is a no-op. --- src/tasks/deploy_tasks.py | 179 +++++++++++++++++++ tests/unit/test_student_membership_gc.py | 214 +++++++++++++++++++++++ 2 files changed, 393 insertions(+) create mode 100644 tests/unit/test_student_membership_gc.py diff --git a/src/tasks/deploy_tasks.py b/src/tasks/deploy_tasks.py index 265dcae..7eda454 100644 --- a/src/tasks/deploy_tasks.py +++ b/src/tasks/deploy_tasks.py @@ -597,6 +597,133 @@ def _fail(repo, log_service, deployment_id: str, error_msg: str) -> dict: +def _gc_orphan_student_memberships(db, user_ids: set[str]) -> None: + """Drop CourseMember / GroupMember / User rows for users that no longer + have any deployment behind them. + + Called from ``delete_deployment`` AFTER the deployment row is gone. For + each candidate user: + 1. Walk their CourseMember rows. + 2. For each CourseMember, count its GroupMember rows that still point + to a course_group with at least one DeploymentInstanceAccess on any + live DeploymentInstance. If zero remain → drop the GroupMembers and + the CourseMember. + 3. Once a user has no CourseMember rows left, drop the User row. + + Limited to users that are pure students. The caller's + ``DeploymentService._sync_student_memberships`` is the only place we + auto-create User rows from a wizard payload, and only for students; + lecturers/admins arrive via Keycloak login (UserSyncService) and + additionally own templates / OpenStack projects, so this function + leaves them alone via the explicit "no owner rows" guard. + """ + if not user_ids: + return + + from src.models.course_member import CourseMember + from src.models.deployment_instance import DeploymentInstance + from src.models.deployment_instance_access import DeploymentInstanceAccess + from src.models.group_member import GroupMember + from src.models.openstack_project import OpenstackProject + from src.models.template import Template + from src.models.user import User + + removed_users = 0 + removed_course_members = 0 + removed_group_members = 0 + + for user_id in user_ids: + # Guard: skip anyone who owns templates or openstack projects — + # cannot be a pure student. This is the safety net against + # accidentally pruning a lecturer if user_ids ever contains one. + owns_templates = ( + db.query(Template.id).filter(Template.owner_id == user_id).first() + is not None + ) + owns_openstack = ( + db.query(OpenstackProject.id) + .filter(OpenstackProject.owner_user_id == user_id) + .first() + is not None + ) + if owns_templates or owns_openstack: + continue + + course_members = db.query(CourseMember).filter( + CourseMember.user_id == user_id + ).all() + + for cm in course_members: + # Which group_memberships of this course_member still point to a + # group that has any live DeploymentInstanceAccess? + live_group_member_ids = { + row[0] + for row in db.query(GroupMember.id) + .join( + DeploymentInstanceAccess, + DeploymentInstanceAccess.group_id == GroupMember.group_id, + ) + .join( + DeploymentInstance, + DeploymentInstance.id + == DeploymentInstanceAccess.deployment_instance_id, + ) + .filter(GroupMember.course_member_id == cm.id) + .distinct() + .all() + } + + stale_group_members = ( + db.query(GroupMember) + .filter(GroupMember.course_member_id == cm.id) + .all() + ) + for gm in stale_group_members: + if gm.id in live_group_member_ids: + continue + db.delete(gm) + removed_group_members += 1 + + # After pruning, does this CourseMember have any live group + # memberships left? If not, drop the CourseMember itself. + db.flush() + remaining = ( + db.query(GroupMember.id) + .filter(GroupMember.course_member_id == cm.id) + .first() + ) + if remaining is None: + db.delete(cm) + removed_course_members += 1 + + db.flush() + # If no CourseMember rows survived → user is no longer tied to ANY + # course → safe to drop. We also re-check owner relationships in case + # something raced. + remaining_cm = ( + db.query(CourseMember.id) + .filter(CourseMember.user_id == user_id) + .first() + ) + if remaining_cm is None: + user = db.query(User).filter(User.id == user_id).first() + if user is not None: + db.delete(user) + removed_users += 1 + + db.commit() + if removed_users or removed_course_members or removed_group_members: + logger.info( + "Student GC complete", + extra={ + "removed_users": removed_users, + "removed_course_members": removed_course_members, + "removed_group_members": removed_group_members, + "candidates": len(user_ids), + }, + ) + + @celery_app.task(bind=True) def delete_deployment(self, deployment_id: str) -> dict: """Delete a deployment's OpenStack resources and remove DB record. @@ -758,6 +885,39 @@ def delete_deployment(self, deployment_id: str) -> dict: "task_id": task_id, } + # Before tearing down DB rows, collect the student users tied to this + # deployment so we can clean them up after the deployment row is gone. + # We snapshot user/course-member ids now — once the DeploymentInstance + # → access → group_id chain is deleted there's no way to walk back to + # the affected students. + student_user_ids: set[str] = set() + try: + from src.models.deployment_instance import DeploymentInstance + from src.models.deployment_instance_access import DeploymentInstanceAccess + from src.models.group_member import GroupMember + from src.models.course_member import CourseMember + + student_user_ids = { + row[0] + for row in db.query(CourseMember.user_id) + .join(GroupMember, GroupMember.course_member_id == CourseMember.id) + .join( + DeploymentInstanceAccess, + DeploymentInstanceAccess.group_id == GroupMember.group_id, + ) + .join( + DeploymentInstance, + DeploymentInstance.id == DeploymentInstanceAccess.deployment_instance_id, + ) + .filter(DeploymentInstance.deployment_id == deployment_id) + .distinct() + .all() + } + except Exception as e: + logger.warning( + f"Failed to snapshot student users for cleanup {deployment_id}: {e}" + ) + # Delete logs first (before deployment record) try: log_repo = DeploymentLogRepository(db) @@ -796,6 +956,25 @@ def delete_deployment(self, deployment_id: str) -> dict: except Exception as e: logger.error(f"Failed to delete deployment record {deployment_id}: {e}", exc_info=True) + # Garbage-collect student users + course/group memberships that no + # longer have ANY deployment behind them. Rationale: + # * Students don't own templates or OpenStack projects (only lecturers + # /admins do), so a user with no remaining CourseMember rows is + # guaranteed to be a former student. + # * Each course_member -> group_member chain that survives here would + # otherwise live forever, growing the membership tables monotonically. + # We re-query the surviving access rows (across ALL other deployments) + # to decide what's safe to drop — a student in two deployments stays + # until the second one is also deleted. + try: + _gc_orphan_student_memberships(db, student_user_ids) + except Exception as e: + logger.warning( + f"Student cleanup failed for deployment {deployment_id}: {e}", + exc_info=True, + ) + db.rollback() + return {"status": "deleted", "deployment_id": deployment_id, "task_id": task_id} except Exception as e: diff --git a/tests/unit/test_student_membership_gc.py b/tests/unit/test_student_membership_gc.py new file mode 100644 index 0000000..a81de58 --- /dev/null +++ b/tests/unit/test_student_membership_gc.py @@ -0,0 +1,214 @@ +"""Tests for _gc_orphan_student_memberships in src.tasks.deploy_tasks. + +The GC runs at the tail end of delete_deployment, AFTER the deployment +row, its instances, and its access rows are gone. It walks every user +id that was tied to the deleted deployment and drops: + + - GroupMember rows whose course_group no longer has any live + DeploymentInstanceAccess + - CourseMember rows whose last GroupMember just vanished + - User rows whose last CourseMember just vanished, provided the user + owns no templates / openstack projects (= guaranteed pure student) + +These tests exercise the helper against an in-memory SQLite DB with the +real schema, so the query joins are vetted as part of the test. +""" +import pytest +from sqlalchemy import create_engine +from sqlalchemy.orm import sessionmaker +from sqlalchemy.pool import StaticPool + +from src.core.database import Base +from src.models.course import Course +from src.models.course_group import CourseGroup +from src.models.course_member import CourseMember +from src.models.deployment import Deployment, DeploymentStatus +from src.models.deployment_instance import DeploymentInstance +from src.models.deployment_instance_access import ( + AccessType, + DeploymentInstanceAccess, +) +from src.models.group_member import GroupMember +from src.models.openstack_project import OpenstackProject +from src.models.template import Template, TemplateVisibility +from src.models.template_version import TemplateVersion +from src.models.user import User +from src.tasks.deploy_tasks import _gc_orphan_student_memberships + + +@pytest.fixture +def db_session(): + """Fresh in-memory SQLite per test with all relevant tables present.""" + # Make sure every model the GC touches is registered on Base.metadata + # before create_all runs. + import src.models.deployment_log # noqa: F401 + import src.models.template_version_file # noqa: F401 + import src.models.template_category # noqa: F401 + import src.models.template_category_assignment # noqa: F401 + + engine = create_engine( + "sqlite:///:memory:", + connect_args={"check_same_thread": False}, + poolclass=StaticPool, + ) + Base.metadata.create_all(engine) + Session = sessionmaker(bind=engine) + session = Session() + try: + yield session + finally: + session.close() + Base.metadata.drop_all(engine) + + +def _scaffold(db, group_name="Group A"): + """Build the smallest set of rows the GC needs: user → course_member → + group_member → course_group. Returns the row references.""" + user = User(external_id="kc-1", username="s1") + db.add(user) + db.flush() + course = Course(name="C", keycloak_course_id="kc-c1") + db.add(course) + db.flush() + group = CourseGroup(course_id=course.id, name=group_name) + db.add(group) + db.flush() + cm = CourseMember(user_id=user.id, course_id=course.id) + db.add(cm) + db.flush() + gm = GroupMember(group_id=group.id, course_member_id=cm.id) + db.add(gm) + db.flush() + return user, course, group, cm, gm + + +def test_gc_drops_user_when_no_access_rows_remain(db_session): + """The end-state of delete_deployment: instances + access rows have + already been deleted. The user is now orphaned and must go.""" + user, *_ = _scaffold(db_session) + + _gc_orphan_student_memberships(db_session, {user.id}) + + # User + course_member + group_member all gone + assert db_session.query(User).filter_by(id=user.id).first() is None + assert db_session.query(CourseMember).count() == 0 + assert db_session.query(GroupMember).count() == 0 + + +def test_gc_keeps_user_when_still_referenced_by_another_deployment(db_session): + """If the student still has a live access row on a different + deployment, the GC leaves all their membership rows alone.""" + user, course, group, cm, gm = _scaffold(db_session) + + # Build a second, surviving deployment chain that the student is on: + # openstack_project → template → template_version → deployment → + # deployment_instance → deployment_instance_access linked to `group`. + op = OpenstackProject( + owner_user_id=user.id, # owner doesn't have to be the student here + openstack_project_id="ks-1", + openstack_project_name="osp", + auth_url="x", + username="u", + password="p", + region_name="r", + ) + tpl = Template( + name="t", owner_id=user.id, repo_url="r", visibility=TemplateVisibility.PRIVATE + ) + db_session.add_all([op, tpl]) + db_session.flush() + tv = TemplateVersion(template_id=tpl.id, version="1.0.0", git_commit_sha="abc") + db_session.add(tv) + db_session.flush() + surviving = Deployment( + name="alive", + template_version_id=tv.id, + course_id=course.id, + openstack_project_id=op.id, + status=DeploymentStatus.RUNNING, + ) + db_session.add(surviving) + db_session.flush() + inst = DeploymentInstance(deployment_id=surviving.id, vm_name="i1") + db_session.add(inst) + db_session.flush() + access = DeploymentInstanceAccess( + deployment_instance_id=inst.id, + access_type=AccessType.SSH, + group_id=group.id, + ) + db_session.add(access) + db_session.commit() + + # ...but because owns_templates is true for this user, the GC bails out + # via the owner-guard. Rerun with a "pure student" id to verify the + # access-survives logic itself: use a second user with no owner rows. + pure_user = User(external_id="kc-2", username="s2") + db_session.add(pure_user) + db_session.flush() + pure_cm = CourseMember(user_id=pure_user.id, course_id=course.id) + db_session.add(pure_cm) + db_session.flush() + db_session.add(GroupMember(group_id=group.id, course_member_id=pure_cm.id)) + db_session.commit() + + _gc_orphan_student_memberships(db_session, {pure_user.id}) + + # pure_user's membership remains because group.id still has a live + # DeploymentInstanceAccess (via the surviving deployment). + assert db_session.query(User).filter_by(id=pure_user.id).first() is not None + assert ( + db_session.query(CourseMember).filter_by(user_id=pure_user.id).count() == 1 + ) + + +def test_gc_skips_users_who_own_templates(db_session): + """Safety net: a user who owns a template is NOT a pure student. GC + must leave them alone even if they have a stale CourseMember row.""" + user, course, group, cm, gm = _scaffold(db_session) + db_session.add( + Template( + name="t", + owner_id=user.id, + repo_url="r", + visibility=TemplateVisibility.PRIVATE, + ) + ) + db_session.commit() + + _gc_orphan_student_memberships(db_session, {user.id}) + + assert db_session.query(User).filter_by(id=user.id).first() is not None + assert db_session.query(CourseMember).count() == 1 + assert db_session.query(GroupMember).count() == 1 + + +def test_gc_skips_users_who_own_openstack_projects(db_session): + """Same safety net for openstack project owners.""" + user, course, *_ = _scaffold(db_session) + db_session.add( + OpenstackProject( + owner_user_id=user.id, + openstack_project_id="ks-1", + openstack_project_name="osp", + auth_url="x", + username="u", + password="p", + region_name="r", + ) + ) + db_session.commit() + + _gc_orphan_student_memberships(db_session, {user.id}) + + assert db_session.query(User).filter_by(id=user.id).first() is not None + + +def test_gc_with_empty_user_set_is_a_noop(db_session): + """Defensive: the caller passes an empty set when its snapshot query + failed. The GC must do nothing.""" + user, *_ = _scaffold(db_session) + + _gc_orphan_student_memberships(db_session, set()) + + assert db_session.query(User).filter_by(id=user.id).first() is not None