diff --git a/submit_ce/domain/event/process.py b/submit_ce/domain/event/process.py index 5be4e1bf..5ff788d4 100644 --- a/submit_ce/domain/event/process.py +++ b/submit_ce/domain/event/process.py @@ -161,6 +161,52 @@ def project(self, submission: Submission) -> Submission: return submission +class StoreZzrm(EventWithSideEffect): + """Write ``00README.json`` into ``src/`` and invalidate the stale tar. + + The 00README build directives are derived from preflight and the + submitter's decisions and written into the source directory as + ``00README.json``. Writing a source file changes the file set the + persisted ``.tar.gz`` was built from, so that package is now + stale (it predates 00README). By running as an + :class:`.EventWithSideEffect`, ``SubmitApi.save`` holds the + submission row lock for the duration of :meth:`execute`, so the + write and the invalidation happen atomically and cannot interleave + with a concurrent upload/delete. See the "critical section" design + note in ``CLAUDE.md``. + + We *delete* the source package rather than rebuild it here, matching + the lazy-invalidation pattern in + ``domain/event/file.py::_common_file_change_execute``: the tar's + consumers (preflight, the Download Package button) rebuild it before + reading, and the finalize path rebuilds it for the QA snapshot. We + deliberately do NOT call ``_common_file_change_execute`` -- that + would also drop the preflight/user_decisions/directives we just + generated, which are still valid for this file set. + """ + + NAME = "store 00README" + NAMED = "stored 00README" + + zzrm: dict + + def validate_pre_lock(self, submission: Submission) -> None: + """The submission must exist to have 00README written.""" + if not submission.submission_id: + raise InvalidEvent( + self, "Cannot store 00README: submission has no id.") + + def execute(self, api: 'SubmitApi', submission: Submission) -> None: + """Write 00README.json to src/ and drop the now-stale tar, under lock.""" + file_store = api.get_file_store() + file_store.store_zzrm(submission.submission_id, self.zzrm) + file_store.delete_source_package(submission.submission_id) + + def project(self, submission: Submission) -> Submission: + """No submission-state change; the side effects are the file writes.""" + return submission + + def _stamp_text_and_link(submission: Submission) -> tuple[str, Optional[str]]: """Build the temporary submission stamp text and link. diff --git a/submit_ce/domain/event/tests/test_store_zzrm.py b/submit_ce/domain/event/tests/test_store_zzrm.py new file mode 100644 index 00000000..ecb571f1 --- /dev/null +++ b/submit_ce/domain/event/tests/test_store_zzrm.py @@ -0,0 +1,95 @@ +"""Tests for the ``StoreZzrm`` event. [SUBMISSION-205] + +Exercise ``validate_pre_lock``/``execute``/``project`` directly with a +hand-rolled fake API (no Flask app or real file store), matching the style +of ``test_oversize_detection.py``. + +The behavior under test: writing ``00README.json`` and invalidating the +now-stale source package must happen as one side effect, and the source +package must be *deleted* (lazy invalidation) rather than rebuilt here. +""" + +from datetime import datetime + +import pytest +from pytz import UTC + +from submit_ce.domain import submission as submod, agent +from submit_ce.domain.event.process import StoreZzrm +from submit_ce.domain.exceptions import InvalidEvent + + +class _FakeStore: + def __init__(self): + self.calls = [] + self.zzrm = {} + + def store_zzrm(self, sid, content): + self.calls.append(("store_zzrm", sid, content)) + self.zzrm[sid] = content + + def delete_source_package(self, sid): + self.calls.append(("delete_source_package", sid)) + + +class _FakeApi: + def __init__(self): + self._store = _FakeStore() + + def get_file_store(self): + return self._store + + +def _user(uid="u1"): + return agent.PublicUser(name="Test User", user_id=uid, + email=f"{uid}@example.org", endorsements=[]) + + +def _submission(sid="1234567"): + u = _user() + s = submod.Submission(creator=u, owner=u, created=datetime.now(UTC)) + s.submission_id = sid + return s + + +def test_execute_writes_zzrm_then_deletes_package(): + """execute writes 00README then drops the stale tar, in that order.""" + s = _submission() + api = _FakeApi() + e = StoreZzrm(creator=s.creator, zzrm={"process": {"compiler": "pdflatex"}}) + + e.execute(api, s) + + assert api._store.zzrm[s.submission_id] == {"process": {"compiler": "pdflatex"}} + # store_zzrm must precede delete_source_package (write, then invalidate). + assert api._store.calls == [ + ("store_zzrm", s.submission_id, {"process": {"compiler": "pdflatex"}}), + ("delete_source_package", s.submission_id), + ] + + +def test_execute_does_not_rebuild_package(): + """We invalidate lazily -- no write_source_package/build here.""" + s = _submission() + api = _FakeApi() + e = StoreZzrm(creator=s.creator, zzrm={"a": 1}) + + e.execute(api, s) + + called = {c[0] for c in api._store.calls} + assert "write_source_package" not in called + assert "build_source_package" not in called + + +def test_project_is_noop(): + """The side effect is the file write; the submission state is unchanged.""" + s = _submission() + e = StoreZzrm(creator=s.creator, zzrm={"a": 1}) + assert e.project(s) is s + + +def test_validate_pre_lock_requires_submission_id(): + s = _submission(sid=None) + e = StoreZzrm(creator=s.creator, zzrm={"a": 1}) + with pytest.raises(InvalidEvent): + e.validate_pre_lock(s) diff --git a/submit_ce/ui/controllers/new/final.py b/submit_ce/ui/controllers/new/final.py index 61016f1f..8bdf995f 100644 --- a/submit_ce/ui/controllers/new/final.py +++ b/submit_ce/ui/controllers/new/final.py @@ -16,6 +16,7 @@ from submit_ce.ui.auth import user_and_client_from_session from submit_ce.domain.event import FinalizeSubmission +from submit_ce.domain.event.process import BuildSourcePackage from submit_ce.domain.exceptions import SaveError from submit_ce.ui.controllers.util import validate_command from submit_ce.ui.routes.flow_control import ready_for_next, stay_on_this_stage @@ -27,6 +28,33 @@ Response = Tuple[Dict[str, Any], int, Dict[str, Any]] # pylint: disable=C0103 +def _rebuild_source_package_for_qa(submission_id: str, submitter, client) -> None: + """Rebuild ``.tar.gz`` under the lock before the QA snapshot reads it. + + The QA snapshot (``get_qa_artifact_info``) references the persisted + source package directly and does NOT build one, so if the tar is + stale or absent at finalize the snapshot ships a stale/omitted + source. Dispatching ``BuildSourcePackage`` rebuilds it inside the + submission row lock (see the critical-section note in ``CLAUDE.md``) + so the snapshot captures current source, including ``00README.json``. + [SUBMISSION-205] + + Best-effort and gated on QA being enabled: a failure here must never + fail the submission itself. + """ + from submit_ce.ui.config import settings + if not (settings.QA_GS_UPLOAD_ENABLED or settings.QA_PUBSUB_ENABLED): + return + try: + current_app.api.save( + BuildSourcePackage(creator=submitter, client=client), + submission_id=submission_id, + ) + except Exception: # noqa: BLE001 - QA rebuild must not break finalize + logger.exception( + "Failed to rebuild source package for QA snapshot %s", submission_id) + + def _upload_qa_metadata(file_store, submission_id: str) -> None: """Build and upload the QA submission-snapshot meta.json after finalize. @@ -68,7 +96,7 @@ def _pubsub_qa_metadata(submission_id: str) -> None: def finalize(method: str, params: MultiDict, session: Session, submission_id: str, **kwargs) -> Response: - submitter, _ = user_and_client_from_session(session) + submitter, client = user_and_client_from_session(session) logger.debug(f'method: {method}, ui-app: {submission_id}. {params}') submission, submission_events = get_submission(submission_id) @@ -130,6 +158,7 @@ def finalize(method: str, params: MultiDict, session: Session, except SaveError as e: logger.error('Could not save primary event') raise InternalServerError(response_data) from e + _rebuild_source_package_for_qa(submission_id, submitter, client) _upload_qa_metadata(file_store, submission_id) _pubsub_qa_metadata(submission_id) return ready_for_next((response_data, status.OK, {})) diff --git a/submit_ce/ui/controllers/new/review.py b/submit_ce/ui/controllers/new/review.py index 3aee3af3..e3e07aae 100644 --- a/submit_ce/ui/controllers/new/review.py +++ b/submit_ce/ui/controllers/new/review.py @@ -13,6 +13,7 @@ SetDirectivesAndCleanup, StartPreflight, StartDirectives, # noqa: F401 (StartDirectives used below) + StoreZzrm, ) from submit_ce.domain.event import SetSourceFormat from ...auth import user_and_client_from_session @@ -211,7 +212,14 @@ def review_files(method: str, params: MultiDict, session: Session, zzrm.from_dict(user_decisions_data) zzrm.update_from_preflight(PreflightResponse(**preflight_data)) logger.warning("ZeroZeroReadMe after update_from_preflight: %s", zzrm.to_json()) - current_app.api.get_file_store().store_zzrm(submission_id, zzrm.to_dict()) + # Write 00README.json under the submission row lock and invalidate + # the now-stale source package, rather than a bare (unlocked) + # store_zzrm call. See StoreZzrm and the critical-section note in + # CLAUDE.md. [SUBMISSION-205] + current_app.api.save( + StoreZzrm(creator=submitter, client=client, zzrm=zzrm.to_dict()), + submission_id=submission_id, + ) return ready_for_next((rdata, status.OK, {})) diff --git a/submit_ce/ui/controllers/new/tests/test_final.py b/submit_ce/ui/controllers/new/tests/test_final.py new file mode 100644 index 00000000..adbdc550 --- /dev/null +++ b/submit_ce/ui/controllers/new/tests/test_final.py @@ -0,0 +1,60 @@ +"""Tests for the finalize-time QA source-package rebuild. [SUBMISSION-205] + +The QA snapshot (``get_qa_artifact_info``) references the persisted +``.tar.gz`` directly and never builds one, so finalize must rebuild it +under the lock (via ``BuildSourcePackage``) before the snapshot is taken -- +otherwise QA ships a stale or missing source package. +""" + +from unittest.mock import MagicMock + +from submit_ce.domain import agent +from submit_ce.ui.controllers.new import final +from submit_ce.domain.event.process import BuildSourcePackage +from submit_ce.ui.config import settings + + +def _creator(uid="u1"): + # A real agent -- BuildSourcePackage validates creator/client as + # pydantic discriminated unions, so a bare MagicMock won't construct. + return agent.PublicUser(name="Test User", user_id=uid, + email=f"{uid}@example.org", endorsements=[]) + + +def test_rebuild_dispatches_build_source_package_when_qa_enabled(app, mocker): + """With QA upload enabled, a BuildSourcePackage is dispatched via save().""" + mock_save = mocker.patch.object(app.api, 'save', + return_value=(MagicMock(), [])) + mocker.patch.object(settings, 'QA_GS_UPLOAD_ENABLED', True) + mocker.patch.object(settings, 'QA_PUBSUB_ENABLED', False) + + with app.app_context(): + final._rebuild_source_package_for_qa('123', _creator(), None) + + build_calls = [c for c in mock_save.call_args_list + if c.args and isinstance(c.args[0], BuildSourcePackage)] + assert len(build_calls) == 1 + assert build_calls[0].kwargs['submission_id'] == '123' + + +def test_rebuild_skips_when_qa_disabled(app, mocker): + """With both QA paths disabled, no rebuild is dispatched.""" + mock_save = mocker.patch.object(app.api, 'save') + mocker.patch.object(settings, 'QA_GS_UPLOAD_ENABLED', False) + mocker.patch.object(settings, 'QA_PUBSUB_ENABLED', False) + + with app.app_context(): + final._rebuild_source_package_for_qa('123', _creator(), None) + + mock_save.assert_not_called() + + +def test_rebuild_swallows_errors(app, mocker): + """A rebuild failure must never propagate out of finalize.""" + mocker.patch.object(app.api, 'save', side_effect=RuntimeError("boom")) + mocker.patch.object(settings, 'QA_GS_UPLOAD_ENABLED', True) + mocker.patch.object(settings, 'QA_PUBSUB_ENABLED', True) + + with app.app_context(): + # Must not raise even though save() blows up. + final._rebuild_source_package_for_qa('123', _creator(), None) diff --git a/submit_ce/ui/controllers/new/tests/test_review.py b/submit_ce/ui/controllers/new/tests/test_review.py index e0f171a5..19fb5eb2 100644 --- a/submit_ce/ui/controllers/new/tests/test_review.py +++ b/submit_ce/ui/controllers/new/tests/test_review.py @@ -125,7 +125,8 @@ def test_review_files_post_no_changes_no_preflight_flashes( def test_review_files_post_no_changes_stores_zzrm_and_advances( app, authorized_client, sub_files_tex, mocker): """End-to-end POST: when there are no changes and preflight is present, - the controller stores the merged zzrm and advances to the next stage.""" + the controller dispatches a StoreZzrm event (write 00README under the + submission row lock) and advances to the next stage. [SUBMISSION-205]""" url = f"/{sub_files_tex.submission_id}/review_files" csrf = _get_csrf(authorized_client, url, mocker) @@ -140,17 +141,23 @@ def test_review_files_post_no_changes_stores_zzrm_and_advances( fake_zzrm.to_dict.return_value = {'merged': True} mocker.patch.object(review, 'ZeroZeroReadMe', return_value=fake_zzrm) - mock_store = MagicMock() - mocker.patch.object(app.api, 'get_file_store', return_value=mock_store) + # The 00README write now goes through save() as a StoreZzrm event, not a + # bare (unlocked) store_zzrm file-store call. + mock_save = mocker.patch.object(app.api, 'save', + return_value=(MagicMock(), [])) resp = authorized_client.post(url, data={'csrf_token': csrf, 'action': 'next'}) assert resp.status_code == status.SEE_OTHER fake_zzrm.from_dict.assert_called_once_with({'sources': []}) fake_zzrm.update_from_preflight.assert_called_once() - mock_store.store_zzrm.assert_called_once_with( - str(sub_files_tex.submission_id), {'merged': True} - ) + + zzrm_calls = [c for c in mock_save.call_args_list + if c.args and isinstance(c.args[0], review.StoreZzrm)] + assert len(zzrm_calls) == 1 + event = zzrm_calls[0].args[0] + assert event.zzrm == {'merged': True} + assert zzrm_calls[0].kwargs['submission_id'] == str(sub_files_tex.submission_id) def _make_workspace(*paths):