Skip to content

fix: flush session before upserting session_peers to prevent FK violation#720

Open
TipKnuckle wants to merge 2 commits into
plastic-labs:mainfrom
TipKnuckle:fix/flush-session-before-peer-upsert
Open

fix: flush session before upserting session_peers to prevent FK violation#720
TipKnuckle wants to merge 2 commits into
plastic-labs:mainfrom
TipKnuckle:fix/flush-session-before-peer-upsert

Conversation

@TipKnuckle
Copy link
Copy Markdown

@TipKnuckle TipKnuckle commented May 23, 2026

Description

With autoflush=False in src/db.py, Core-level SQL statements like pg_insert do not automatically flush pending ORM objects before executing. In get_or_create_session, db.add(honcho_session) marks the session as pending, but when _get_or_add_peers_to_session executes the pg_insert upsert on session_peers (which has a foreign key to sessions), the session row doesn't exist in PostgreSQL yet, causing a ForeignKeyViolation.

Fix

Added await db.flush() after the session creation and before the peer upsert, ensuring the session row exists in the database before the FK-dependent upsert executes.

Testing

Verified working with the OWUI sync script that was previously hitting this error (500 from server after a few imports).

Summary by CodeRabbit

  • Bug Fixes
    • Fixed a race condition during session creation that could cause related participant/peer records to fail to attach reliably, improving session setup consistency and reducing intermittent errors when joining or initializing sessions.

Review Change Stack

…tion

With autoflush=False in db.py, Core-level SQL statements like pg_insert
do not automatically flush pending ORM objects. In get_or_create_session,
db.add(honcho_session) marks the session as pending, but when
_get_or_add_peers_to_session executes the pg_insert upsert on
session_peers (which has a FK to sessions), the session row doesn't
exist in PostgreSQL yet, causing a ForeignKeyViolation.

Add await db.flush() after the session creation to ensure the session
row exists before the FK-dependent upsert.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 23, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 81ed1464-8a8b-4d45-a0a5-78d9577184c2

📥 Commits

Reviewing files that changed from the base of the PR and between 847a758 and 4b85fcb.

📒 Files selected for processing (1)
  • src/crud/session.py

Walkthrough

Single-line fix adding explicit db.flush() in get_or_create_session to ensure session rows are persisted before downstream peer upsert operations execute, resolving ORM flush and Core-level INSERT...ON CONFLICT ordering.

Changes

Session creation ordering

Layer / File(s) Summary
Session flush ordering in peer upsert
src/crud/session.py
Explicit await db.flush() is inserted within the nested transaction immediately after creating a new session, ensuring the session row is written to PostgreSQL before _get_or_add_peers_to_session performs upserts on session_peers records that have a foreign key dependency on the session.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A flush at the right time, before peers arrive,
Ensures all the foreign keys stay alive,
Sessions now ordered, no conflicts in sight,
The transaction waits just enough time—all right!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main change: adding a flush before upserting session_peers to prevent foreign key violations.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/crud/session.py`:
- Around line 229-233: The db.flush() call currently sits outside the savepoint
block and must be moved inside the async with db.begin_nested() context so any
IntegrityError during flush is confined to the nested transaction and the outer
transaction remains valid for the recursive retry; modify the flow in the
function where begin_nested() is used (the block that currently calls
_get_or_add_peers_to_session) to call await db.flush() while inside that async
with db.begin_nested():, then proceed to call _get_or_add_peers_to_session,
ensuring IntegrityError from flush triggers the nested rollback only and
preserves the retry behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 181f13dc-9a1e-40d1-ad29-2a670d2e3165

📥 Commits

Reviewing files that changed from the base of the PR and between 7470866 and 847a758.

📒 Files selected for processing (1)
  • src/crud/session.py

Comment thread src/crud/session.py Outdated
Ensures any IntegrityError during flush is confined to the nested
transaction so the outer transaction stays valid for the retry path.
@TipKnuckle
Copy link
Copy Markdown
Author

Good catch — moved db.flush() inside the begin_nested() context in 4b85fcb.

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