SUBMISSION-205: 00README update requires source package refresh

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 ``<id>.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.
 

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)

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 ``<id>.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, {}))

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, {}))
 

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
+``<id>.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)

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):