Skip to content

🪲 BUG-#36: Fix race condition in SQLite save()#55

Merged
FernandoCelmer merged 4 commits into
developfrom
feature/36
Apr 16, 2026
Merged

🪲 BUG-#36: Fix race condition in SQLite save()#55
FernandoCelmer merged 4 commits into
developfrom
feature/36

Conversation

@FernandoCelmer
Copy link
Copy Markdown
Member

Summary

  • Add UniqueConstraint("uid", "mailbox") to RawModel for explicit upsert support
  • Replace check-then-insert pattern in StorageSQLite.save() with SQLite's INSERT OR REPLACE via sqlalchemy.dialects.sqlite.insert and on_conflict_do_update
  • Eliminates race condition where concurrent syncs could duplicate records

Closes #36

Copy link
Copy Markdown
Member Author

@FernandoCelmer FernandoCelmer left a comment

Choose a reason for hiding this comment

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

🔍 Code Review

Code issues found: 1

See inline comment below.

stmt = sqlite_insert(RawModel).values(
message_id=raw.message_id,
uid=raw.uid,
mailbox=raw.mailbox,
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

[Suggestion]

Problem — With SQLite's INSERT ... ON CONFLICT DO UPDATE, result.rowcount returns 1 for both inserts and updates (any affected row = 1). This means save() now always returns True, never False for updates.

The docstring still says "Returns True if inserted, False if updated", which is now incorrect.

Failure scenario

storage = StorageSQLite()
storage.save(raw)          # -> True (inserted) OK
storage.save(raw)          # -> True (updated, but claims inserted) WRONG

This directly affects sync.py:_save_one() which uses the return value to count result.inserted vs result.updated. After this PR, result.updated will always be 0.

Fix — Either update the docstring and _save_one() to not rely on the distinction, or check existence before the upsert:

existing = session.query(RawModel.uid).filter(
    RawModel.uid == raw.uid, RawModel.mailbox == raw.mailbox
).first()
session.execute(stmt)
session.commit()
return existing is None  # True if new, False if updated

@FernandoCelmer FernandoCelmer merged commit 1dd12a9 into develop Apr 16, 2026
11 checks passed
@FernandoCelmer FernandoCelmer deleted the feature/36 branch April 16, 2026 22:03
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.

1 participant