ref(db): Replace create_or_update with update_or_create in Buffer.process#117513
ref(db): Replace create_or_update with update_or_create in Buffer.process#117513vgrozdanic wants to merge 1 commit into
Conversation
d0e326c to
7d8cb69
Compare
…cess The `elif model:` branch in `Buffer.process` was the last remaining caller of Sentry's custom `create_or_update` utility in non-manager production code. A simple `update_or_create` cannot be used here because `update_kwargs` contains `F()` increment expressions that Django would try to evaluate on the INSERT path and fail. Instead, use the explicit pattern: `filter().update()` first; if no rows were affected, `create()` with raw column values inside a savepoint, retrying as an update on `IntegrityError` in case a concurrent writer beat us to the insert. All models that reach this branch — `GroupTombstone`, `ReleaseProject`, `ReleaseProjectEnvironment`, and `GroupRelease` — have unique constraints on their filter fields, so the create-with-retry pattern is safe. Adds regression tests covering the update path (F() increment applied, signal fires with `created=False`), the create path (initial value set, signal fires with `created=True`), and the race-condition path (IntegrityError swallowed, no exception propagated). Co-Authored-By: Claude <noreply@anthropic.com>
7d8cb69 to
9670c9e
Compare
| # only rolls back this attempt, not any outer transaction. | ||
| with transaction.atomic(using=router.db_for_write(model)): | ||
| model.objects.create(**create_kwargs) | ||
| created = True |
There was a problem hiding this comment.
extra fields silently dropped on INSERT path in Buffer.process
In the elif model: rollout branch, when filter().update() matches no rows and a new record is created, create_kwargs is built from filters and columns only — extra is omitted. The legacy create_or_update it replaces did include extra on the INSERT path (it resolved both F-expressions and plain values from values/defaults into create_kwargs). As a result, caller-supplied fields like last_seen are silently dropped on creation and fall back to model defaults. The create branch only fires when the filtered row is absent (e.g. a GroupTombstone/GroupRelease row deleted between the buffer enqueue and flush), so this is a narrow correctness regression rather than a crash.
Evidence
create_or_updateindb/models/query.pybuildscreate_kwargsfromkwargsplus iteratingvalues.items()anddefaults.items(), so non-Fextravalues (e.g.last_seen) were written on INSERT.- The new branch in
buffer/base.pysetscreate_kwargs = {**filters, **{col: v for col, v in columns.items()}}, omittingextra, solast_seenis not passed tomodel.objects.create(). GroupTombstonecaller inevent_manager.py:320-331passesextra={'last_seen': max(event.datetime, group_tombstone.last_seen)}; if the tombstone row is gone at flush time the create path fires andlast_seenfalls back toauto_now_add.ReleaseProject/ReleaseProjectEnvironmentcallers pass noextra, andGroupReleasefilters by an existingid, so the create path withextrais only reached on a deletion race — limiting real-world impact.
Identified by Warden sentry-backend-bugs · C5Q-Q33
Buffer.processhad one remaining caller of Sentry's legacycreate_or_updatein theelif model:branch — the path taken for non-Groupmodels likeReleaseProject,ReleaseProjectEnvironment,GroupTombstone, andGroupRelease.A simple
update_or_createdoesn't work here becauseupdate_kwargsis built fromF()increment expressions (F("new_groups") + 1). Django evaluatesdefaultson both the UPDATE and INSERT paths, and F-expressions fail on INSERT since there is no existing row to resolve against. This is what caused the original migration attempt (reverted in #97840) to fail.Instead this PR uses the explicit pattern:
filter().update()first; if no rows were affected,create()with raw column values inside a savepoint, retrying as a plain update onIntegrityErrorin case a concurrent worker inserts between the two steps. All models that reach this branch have unique constraints on their filter fields, so the create-with-retry pattern is safe.Regression tests added for the update path (F() increment applied,
buffer_incr_completesignal fires withcreated=False), the create path (initial value set, signal fires withcreated=True), and the race-condition path (IntegrityErroron create is swallowed, no exception propagates). Also removes the stalecreate_or_updatemock fromtest_signal_onlyand removessrc/sentry/buffer/base.pyfrom the allowlist.Part of the broader
create_or_updatedeprecation tracked in the repo.