Skip to content

Conversation

@spalmurray
Copy link
Contributor

Replaces PostgreSQL's auto-increment sequence with a counter table approach to ensure incident IDs increment exactly by one with no gaps. The counter row is locked during incident creation so that failed saves roll back the counter increment.

@spalmurray spalmurray marked this pull request as ready for review January 16, 2026 21:18
def get_next_incident_id() -> int:
"""Atomically get and increment the incident ID counter."""
with transaction.atomic():
counter = IncidentCounter.objects.select_for_update().get()
Copy link

Choose a reason for hiding this comment

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

Bug: The get_next_incident_id function will crash if the single row in the IncidentCounter table is missing or duplicated, as there are no safeguards.
Severity: HIGH

Suggested Fix

Strengthen the get_next_incident_id function to be resilient to a missing counter row. Use update_or_create or a similar pattern to ensure the counter row exists before attempting to read and increment it. This prevents crashes if the row is accidentally deleted.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/firetower/incidents/models.py#L24

Potential issue: The `get_next_incident_id` function retrieves the next incident ID by
calling `IncidentCounter.objects.select_for_update().get()`. This operation assumes
exactly one row exists in the `incidents_incident_counter` table. However, there are no
database-level constraints or application-level safeguards to prevent this row from
being accidentally deleted or duplicated through manual database operations. If the row
is missing, any attempt to create an incident will raise an unhandled
`IncidentCounter.DoesNotExist` exception, blocking all new incident creation. If the row
is duplicated, it will raise `MultipleObjectsReturned`.

Did we get this right? 👍 / 👎 to inform future reviews.

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.

2 participants