What happens
buzz.api.proposals.accept_proposal reads whether an Event Talk already exists for a
proposal, then calls TalkProposal.create_talk() if not. Nothing holds a lock between the
two, and Event Talk.validate repeats the same unlocked read, so two reviewers accepting
the same proposal in the same request window can both see no talk and both create one.
Event Talk.proposal has no unique: 1, so the database does not catch it either.
Why it isn't urgent
Two people have to press Accept on the same proposal within the same few hundred
milliseconds — the drawer disables its own button while saving, so it is a cross-user race
only. The result is two programme rows for one proposal, cleaned up by deleting one. No
data is lost.
Worth noting the direction: before #409, create_talk never wrote talk.proposal at all,
so the duplicate guard in Event Talk.validate had never fired and duplicates needed no
race whatsoever. #409 stamped the link and closed the common case. This issue is the
remainder.
Fix
Set unique: 1 on Event Talk.proposal and let the database be the guard, catching
frappe.exceptions.DuplicateEntryError in accept_proposal to return the talk that won
the race. Needs a patch that checks for pre-existing duplicate rows before the index goes
on; rows with a null proposal are fine, since MySQL allows repeated nulls in a unique
index.
Found by Greptile on #409.
What happens
buzz.api.proposals.accept_proposalreads whether anEvent Talkalready exists for aproposal, then calls
TalkProposal.create_talk()if not. Nothing holds a lock between thetwo, and
Event Talk.validaterepeats the same unlocked read, so two reviewers acceptingthe same proposal in the same request window can both see no talk and both create one.
Event Talk.proposalhas nounique: 1, so the database does not catch it either.Why it isn't urgent
Two people have to press Accept on the same proposal within the same few hundred
milliseconds — the drawer disables its own button while saving, so it is a cross-user race
only. The result is two programme rows for one proposal, cleaned up by deleting one. No
data is lost.
Worth noting the direction: before #409,
create_talknever wrotetalk.proposalat all,so the duplicate guard in
Event Talk.validatehad never fired and duplicates needed norace whatsoever. #409 stamped the link and closed the common case. This issue is the
remainder.
Fix
Set
unique: 1onEvent Talk.proposaland let the database be the guard, catchingfrappe.exceptions.DuplicateEntryErrorinaccept_proposalto return the talk that wonthe race. Needs a patch that checks for pre-existing duplicate rows before the index goes
on; rows with a null
proposalare fine, since MySQL allows repeated nulls in a uniqueindex.
Found by Greptile on #409.