Repository navigation
service-template-897: One committed command, one emission, under concurrency - #75
Merged
Merged
Conversation
The core jobs contract published job_fired inside the still-open request transaction and decided whether to emit from a row read before any lock, so a failed commit left a behaviour executed that no command recorded, and two concurrent retries of one undelivered command both emitted. The controller now records the command, commits it, and only then reaches the single named place that emits: _emit_once, which takes the committed row's lock (SELECT ... FOR UPDATE on PostgreSQL), decides from the row that statement returns, emits, marks dispatch terminal and commits. A waiting retry therefore reads the winner's terminal evidence instead of emitting beside it. The lock is released by the commit that records the evidence, so the surviving hazard is a second emission after a crash between delivery and that commit, never a command marked dispatched that never emitted. The backend suite runs on SQLite, which ignores FOR UPDATE, so the row lock's mutual exclusion is shown by compiling the statement against the PostgreSQL and SQLite dialects rather than by a green test. The new tests exhibit the two failures: a commit that fails after the point where the old code had already emitted publishes nothing, and a retry that reaches the lock after another retry completed emits nothing. The test fixture now joins the outer transaction with create_savepoint so application code can commit; no contract, schema, route, manifest, migration or contract_version changed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Card:
service-template-897— One committed command, one emission, under concurrencyBranch:
pipeline/service-template-897→mainWhat the card asks for
Goal
Close the two windows in the released core jobs contract where a fire can execute a behaviour that
no committed command records, or execute it twice. The sprint's Definition of Done requires that
command identity and terminal evidence survive retry and that replay cannot create an unbounded
duplicate execution;
service-template-896delivered that for sequential retries, and these are thetwo paths where it does not yet hold.
Both were found by the reviewer of 896 and neither violated its acceptance criteria — they are
released behaviour now, and they are cheaper to close before any product provides
jobs.firethanafter.
Context
Repository:
/home/dev/projects/service-template,mainatd4b07a5(896 merged)./home/dev/projects/codegen_orchestratoris not touched by this card, and neither isscheduler.service_template_ref— the pin moves in the next card, to this card's candidate.The released code:
template/services/backend/src/controllers/jobs.py::JobsController.fire— validates, callsrepository.record(...), returns early when the command is alreadyDISPATCHED, otherwise calls_emit(command)and, on success,repository.mark_dispatched(...).template/services/backend/src/app/repositories/job_command.py—get,record(read, theninsert inside
begin_nested(),IntegrityError→ re-read),mark_dispatched(a flush, nocommit).
get_async_db()after the controller returns, so_emit()publishes while the row is still uncommitted.template/services/backend/src/app/models/job_command.py— identity is the unique(fired_by_product, command_id);DispatchStatusisundelivered/dispatched.template/services/backend/tests/unit/test_jobs.py, whose suite runs onsqlite+aiosqlite(template/services/backend/tests/conftest.py). The product's real database isPostgreSQL.
The invariant
A
job_firedevent exists only for a command whose row is committed, and at most one emissionever happens per command identity — however many callers fire or retry it, and however they
interleave.
Enforce it in one named place and make every path reach it. The two known violations:
_emit()publishes inside the still-open request transaction. If thecommit then fails, the behaviour has been executed and no command records it, so the next retry
executes it again — and each further failed commit repeats that.
undeliveredcommand. Both callers read the same non-terminal row,neither meets the unique constraint (the row already exists), and both emit.
Acceptance criteria
job_firedis published for a command whose row is not durably committed. Ordering, nothope: the command is committed first, and the emission happens after, so a failure between them
leaves a recorded command that a retry can complete rather than an execution nobody recorded.
report — a row lock taken on the emit path, or a conditional terminal transition whose winner is
decided by the database — and say which hazard your choice accepts: a double emission is what the
card exists to remove, but a command marked terminal that never emitted would be a lost
execution wearing trustworthy evidence, which is worse. Name which one your design can still
produce, in what window, and why that is the safe side.
silently makes some locking a no-op, so state plainly in the report what the suite proves and
what it cannot: if the mechanism depends on PostgreSQL semantics, show that the statement the
ORM emits is the one you intend (compiled SQL, dialect-aware), and do not present a green SQLite
test as proof of mutual exclusion.
… (truncated; the full text is on the card)
What the worker reports
service-template-897 — jobs: emit only what is committed, and only once
Branch
pipeline/service-template-897, one commit6dc680eon top ofd4b07a5. Working treeclean. No contract, schema, route, manifest, migration or
contract_versionchanged.What changed
template/services/backend/src/controllers/jobs.pyJobsController.firenow: validates →repository.record(...)→session.commit()→ handsoff to one named place,
_emit_once(repository, fired_by_product, command_id)._emit_onceis the only place ajob_firedis published and the only place dispatch becomesterminal. It takes the committed row's lock, decides from the row that statement returns, emits,
marks dispatched on a reported delivery, builds the contract, and commits (releasing the lock).
template/services/backend/src/app/repositories/job_command.pydispatch_lock_statement(fired_by_product, command_id):select(JobCommand).where(identity).with_for_update().execution_options(populate_existing=True),and
JobCommandRepository.lock_for_dispatch(...)which executes it. It is a module-level functionprecisely so a test can compile it dialect-aware without a database.
populate_existingis there for the real deployment: a waiting request has its own session, andwithout it SQLAlchemy would hand back the copy that session had already loaded instead of the row
the winner committed.
template/services/backend/tests/conftest.pyjoin_transaction_mode="create_savepoint",and the
get_async_dboverride yields the session directly instead of wrapping the request in asavepoint. That is the documented SQLAlchemy 2.0 recipe for code that commits inside the request;
the outer transaction still rolls every test's writes back. No existing assertion was changed or
weakened — this is the fixture learning to host a controller that owns a commit.
Docs:
docs/CONTRACTS.mdgains "Committed before emitted, emitted once" inside Core jobs v1;CHANGELOG.md(English) carries the change under Unreleased/Changed;template/services/backend/AGENTS.mdhad a flat rule "Controllers must not callsession.commit()" —it now names the jobs core as the one deliberate exception and says why, because leaving it would
have made the shipped guidance false.
AC2 — the mechanism, and the hazard it accepts
Chosen: a row lock taken on the emit path (
SELECT ... FOR UPDATEon the committed row), not aconditional terminal transition.
Why: the ordering must stay emit, then mark terminal. A conditional
UPDATE ... WHERE status = 'undelivered'that claims the command before publishing would let the database pick a uniquewinner, but the winner could then fail to deliver, or die, leaving a row marked
dispatchedwithdispatched_atset and no event ever published — a lost execution wearing trustworthy evidence,which the card names as the worse outcome and which central QA would read as proof the behaviour
ran. The row lock gets mutual exclusion without ever inverting that order.
Hazard this design can still produce: a second emission, in exactly one window — after
publish_job_firedhas returned and before the commit that recordsdispatchedsucceeds (processdeath, connection loss, a failing commit). The command stays
undelivered, so a later retry emitsagain. That is at-least-once on a bounded, self-healing path: each retry still records no second
command, and the number of executions is bounded by the number of retries, not unbounded. The
impossible direction is the important one — the row is never marked
dispatchedbefore delivery hasbeen reported, so a command carrying terminal evidence always had its event published.
Cost accepted knowingly: the lock is held across the publish, so a slow broker holds one row lock
per command identity. It serialises only retries of the same identity; different commands never
contend.
… (truncated; the full text is on the card)
Opened by the secretary CI gate so that the
pull_requestworkflow runs. Title and body are built from the card and the worker's done report.