Skip to content

This change adds a SaveParticipant abstraction#82

Open
bdc34 wants to merge 6 commits into
developfrom
bdc34/submitapi-participants
Open

This change adds a SaveParticipant abstraction#82
bdc34 wants to merge 6 commits into
developfrom
bdc34/submitapi-participants

Conversation

@bdc34

@bdc34 bdc34 commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Goal
To allow the SubmitAPI to have more options to add behavior that happens inside protected critical sections.

This changes provides several new places to tie in code that is run inside the transaction that makes up the protected critical section.

Justification
SubmitApi.save() runs a critical section (row lock → validate/execute events → persist → commit) that is currently a private detail of LegacySubmitImplementation. Composed implementations like PubsubEventSubmitImplementation can only act before/after the whole save() call. This is outside the transaction and nothing is notified of rollback.

This adds ability to have behavior in the save transaction with four phases of callbacks:

under_lock just after locking the submission.

before_commit after the events have been executed() and applied()

after_commit after commit to the db.

on_rollback On thrown exceptions. on_rollback receives an explicit SaveFailure record (exception + SavePhase enum + current event/participant) built from a cursor the save loop maintains, so participants never reverse-engineer failure location from exception types.

A SaveParticipants is injected via constructor to the SubmitApi.

Scope: core machinery + tests only. Pubsub stays a decorator; conversion in follow-up work.

bdc34 and others added 2 commits July 8, 2026 14:25
Goal to allow the SubmitAPI to have more options to add behavior
that happens inside protected critical sections.

Adds ability to have behavior in the save transaction with four phases of
callbacks (`under_lock`, `before_commit`, `after_commit`, `on_rollback`),
injected via constructor. `on_rollback` receives an explicit `SaveFailure`
record (exception + `SavePhase` enum + current event/participant) built from a
cursor the save loop maintains, so participants never reverse-engineer failure
location from exception types.

`SubmitApi.save()` runs a critical section (row lock → validate/execute events →
persist → commit) that is currently a private detail of
`LegacySubmitImplementation`.

Composed implementations like `PubsubEventSubmitImplementation` can only act
before/after the whole `save()` call. This is *outside* the transaction and
nothing is notified of rollback.

This changes provides several new places to tie in code that is run inside the
transaction that makes up the protected critical section.

Scope: **core machinery + tests only**. Pubsub stays a decorator; conversion in
follow-up work.
Resolve two semantic conflicts between the SaveParticipant refactor and
develop's SUBMISSION-169 (build source package under lock):

- _FakeStore in test_oversize_detection.py: add no-op
  delete_source_package/delete_user_decisions/delete_directives, which
  _common_file_change_execute now calls.
- BuildSourcePackage: rename validate -> validate_pre_lock to match the
  renamed base-class hook that EventWithSideEffect.apply invokes.

Co-Authored-By: Claude Fable 5 <[email protected]>
@bdc34
bdc34 marked this pull request as ready for review July 9, 2026 20:14

@norbusan norbusan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, no time to read through all this now, but I asked codex GPT/Sol and it gave only one issues:

   - P2: Consequence failures are reported as persistence failures. legacy_implementation/__init__.py:301

    (https://github.com/arXiv/submit-ce/blob/99d96702c7b007fe3326e826b734f228c9672aca/submit_ce/implementations/legacy_implementation/__init__.py#L301)
    calls event.get_consequences() while ctx.phase remains EVENT_PERSIST. That method deliberately raises for undeclared consequence
    types, so rollback participants receive an inaccurate SaveFailure.phase, contrary to the abstraction’s “exact failure location”
    contract. Add an EVENT_CONSEQUENCES phase and set it before this call, with a regression test.

@bmaltzan

Copy link
Copy Markdown
Contributor

I added a test, showing the rollback might possibly mask a previous error.

@bdc34

bdc34 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Sorry, no time to read through all this now, but I asked codex GPT/Sol and it gave only one issues:

   - P2: Consequence failures are reported as persistence failures. legacy_implementation/__init__.py:301

    (https://github.com/arXiv/submit-ce/blob/99d96702c7b007fe3326e826b734f228c9672aca/submit_ce/implementations/legacy_implementation/__init__.py#L301)
    calls event.get_consequences() while ctx.phase remains EVENT_PERSIST. That method deliberately raises for undeclared consequence
    types, so rollback participants receive an inaccurate SaveFailure.phase, contrary to the abstraction’s “exact failure location”
    contract. Add an EVENT_CONSEQUENCES phase and set it before this call, with a regression test.

I'll add this.

bdc34 added 2 commits July 23, 2026 14:53
vent.get_consequences() is called while ctx.phase remains EVENT_PERSIST. That
method deliberately raises for undeclared consequence types, so rollback
participants receive an inaccurate SaveFailure.phase, contrary to the
abstraction’s “exact failure location” contract.

Adds an EVENT_CONSEQUENCES phase and set it before this call.

Adds a test.
@bdc34

bdc34 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Consequence failures are reported as persistence failures. legacy_implementation/init.py:301

(https://github.com/arXiv/submit-ce/blob/99d96702c7b007fe3326e826b734f228c9672aca/submit_ce/implementations/legacy_implementation/__init__.py#L301)
calls event.get_consequences() while ctx.phase remains EVENT_PERSIST. That method deliberately raises for undeclared consequence
types, so rollback participants receive an inaccurate SaveFailure.phase, contrary to the abstraction’s “exact failure location”
contract. Add an EVENT_CONSEQUENCES phase and set it before this call, with a regression test.

Fixed

@bdc34

bdc34 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

I added a test, showing the rollback might possibly mask a previous error.

Interesting case, I'm looking into it.

@bdc34

bdc34 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

I added a test, showing the rollback might possibly mask a previous error.

Interesting case, I'm looking into it.

Claude reports that session.__exit__() which is called at the end of the with session: ... block can also raise.

@bdc34

bdc34 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

I added a test, showing the rollback might possibly mask a previous error.

This is fixed.

Added a rolledback parameter to SaveFailure https://github.com/arXiv/submit-ce/pull/82/changes#diff-b840e6b1c88d21f195eb180d757a50df8b09a8616971314afd2fac967713a4eaR124

Put the rollback in a try
https://github.com/arXiv/submit-ce/pull/82/changes#diff-e944c3ea3f8e7fa54c85450d0a927336de2785f74f2eba99d5f4f7a4e682525fR206

@bdc34
bdc34 requested a review from norbusan July 23, 2026 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants