fix: session commit tracking, Windows event loop, and DOB coercion - #44
Conversation
- Add needs_commit session flag to track post-flush uncommitted DML (fixes flatmate listings vanishing after create returned 200) - Add explicit commit in create_property after prescreen flush - Add explicit commit in update_user to prevent auth-state race - Coerce date_of_birth from date to UTC datetime for ORM column - Set WindowsSelectorEventLoopPolicy on win32 for uvicorn compatibility - Add tests for flush-commit tracking and DOB coercion
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChangesDatabase commit tracking
Service persistence and date handling
Windows runtime startup
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/unit/core/test_database_session.py (1)
119-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the flushed background-session path.
get_bg_db()now has the same flushed-state commit branch, but coverage only exercisesget_db(). Add aflushed=Truetest usingAsyncSessionLocalBGand assert one commit.🤖 Prompt for 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. In `@tests/unit/core/test_database_session.py` around lines 119 - 140, The test currently covers only the flushed commit path in get_db; add a parallel async test for get_bg_db using AsyncSessionLocalBG, configure the fake session with pending=False and flushed=True, exhaust the generator, and assert that commit is awaited exactly once.
🤖 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 `@app/core/database.py`:
- Around line 287-289: Update the _mark_session_needs_commit function signature
to annotate _flush_context, using sqlalchemy.orm.InstanceState[object] when its
type is known or Any when the context is intentionally opaque; preserve the
existing session.info assignment.
In `@app/services/property/crud.py`:
- Around line 285-290: Move creation or scheduling of the separate-session
verifier to after the explicit await db.commit() in the property creation flow.
Ensure the verifier only starts once the property and images are committed and
visible, while preserving the existing cleanup behavior and commit handling.
In `@run.py`:
- Around line 95-104: The Windows branch in the startup flow must preserve
Uvicorn’s reload supervision. Update the logic around the Windows-specific
SelectorEventLoop and Server.serve call so reload=True uses Uvicorn’s main
entry-point path, or explicitly reject reload=True on Windows; retain the
existing direct server behavior only when reload is disabled.
---
Nitpick comments:
In `@tests/unit/core/test_database_session.py`:
- Around line 119-140: The test currently covers only the flushed commit path in
get_db; add a parallel async test for get_bg_db using AsyncSessionLocalBG,
configure the fake session with pending=False and flushed=True, exhaust the
generator, and assert that commit is awaited exactly once.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8992d892-aa15-4a3e-9276-4e9ad2d87100
📒 Files selected for processing (7)
app/core/database.pyapp/main.pyapp/services/property/crud.pyapp/services/user.pyrun.pytests/unit/core/test_database_session.pytests/unit/services/test_user_service.py
| @event.listens_for(Session, "after_flush") | ||
| def _mark_session_needs_commit(session: Session, _flush_context) -> None: | ||
| session.info[_SESSION_NEEDS_COMMIT_KEY] = True |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target function and nearby imports in app/core/database.py.
if [ -f app/core/database.py ]; then
echo "== file stats =="
wc -l app/core/database.py
echo "== imports/events section =="
sed -n '1,80p;260,305p' app/core/database.py | cat -n
else
echo "app/core/database.py not found"
fd -a 'database.py' .
fi
echo "== pylint/type annotation references for flush events =="
rg -n "after_flush|_flush_context|Session|listen" app/core/database.pyRepository: 360ghar/360ghar-backend
Length of output: 6677
🌐 Web query:
SQLAlchemy Session after_flush event signature listens_for after_flush FlushContext
💡 Result:
The after_flush event in SQLAlchemy is part of the SessionEvents interface and is triggered after the SQL for a flush process has been emitted to the database, but before the transaction has been committed [1][2][3]. Signature The event listener function should accept the following two arguments [1][2]: def receive_after_flush(session, flush_context): # event handling logic session: The target Session object [4][5]. flush_context: An internal UOWTransaction (Unit of Work Transaction) object that manages the details of the flush process [4][5]. Usage You can subscribe to this event using the @event.listens_for decorator [1][2]. from sqlalchemy import event @event.listens_for(Session, 'after_flush') def receive_after_flush(session, flush_context): # Logic to execute after the flush is complete pass Behavior At the time this event is triggered, the Session's state (such as the new, dirty, and deleted collections) still reflects the pre-flush state [1][2][4]. This allows you to inspect what objects were involved in the flush or use history tracking features to see the changes that were just persisted to the database [3]. You can also emit additional SQL queries to the database within this hook based on the changes you observe [3]. Note that if you need to perform actions after the objects have been updated to reflect the flush (for example, if you need to see that an object's state is no longer 'pending' or 'dirty'), you should use the after_flush_postexec event instead [3].
Citations:
- 1: https://docs.sqlalchemy.org/en/20/orm/events.html
- 2: https://docs.sqlalchemy.org/en/21/orm/events.html
- 3: https://docs.sqlalchemy.org/en/21/orm/session_events.html
- 4: https://docs.sqlalchemy.org/en/14/orm/events.html
- 5: https://docs.sqlalchemy.org/en/14/orm/events.html?highlight=before_flush
Annotate the flush-event context parameter.
_mark_session_needs_commit(...) leaves _flush_context without a type hint, but app/**/*.py requires full type hints. Annotate it as sqlalchemy.orm.InstanceState[object] or use Any if the flush context is intentionally opaque here.
🤖 Prompt for 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.
In `@app/core/database.py` around lines 287 - 289, Update the
_mark_session_needs_commit function signature to annotate _flush_context, using
sqlalchemy.orm.InstanceState[object] when its type is known or Any when the
context is intentionally opaque; preserve the existing session.info assignment.
Source: Coding guidelines
| if sys.platform == "win32": | ||
| import selectors | ||
| selector = selectors.SelectSelector() | ||
| loop = asyncio.SelectorEventLoop(selector) | ||
| asyncio.set_event_loop(loop) | ||
| config = uvicorn.Config("app.main:app", **uvicorn_kwargs) | ||
| server = uvicorn.Server(config) | ||
| loop.run_until_complete(server.serve()) | ||
| else: | ||
| uvicorn.run("app.main:app", **uvicorn_kwargs) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
uv run python - <<'PY'
import inspect
import uvicorn
print(inspect.getsource(uvicorn.run))
print(inspect.getsource(uvicorn.Server.serve))
PYRepository: 360ghar/360ghar-backend
Length of output: 201
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== locate run.py and uvicorn dependency declarations =="
fd -a '^run\.py$' .
printf '\n'
rg -n "uvicorn|reload|Server\.serve|run_until_complete" -S . || true
printf '\n== run.py relevant section if present ==\n'
if [ -f run.py ]; then
nl -ba run.py | sed -n '1,150p'
fi
printf '\n== package files with dependency/version specs =="
for f in pyproject.toml uv.lock requirements.txt Pipfile setup.py setup.cfg poetry.lock; do
[ -f "$f" ] && printf '\n--- %s ---\n' "$f" && sed -n '1,220p' "$f"
doneRepository: 360ghar/360ghar-backend
Length of output: 7676
🌐 Web query:
Uvicorn 0.46.0 uvicorn.run ChangeReload Server.serve source
💡 Result:
Uvicorn version 0.46.0, released on April 23, 2026, introduced changes related to WebSocket implementation and internal buffer management, but it did not introduce changes to the fundamental relationship or API usage between uvicorn.run and Server.serve [1][2][3]. The relationship between these methods remains established as follows: 1. uvicorn.run: This is the standard, high-level entry point designed to be a programmatic equivalent of the Uvicorn command-line interface [4][5]. It handles the configuration and execution of the server, including setting up the event loop and managing the server lifecycle [6][7]. 2. Server.serve: This is a low-level method available on the uvicorn.Server instance [6][4]. It is intended for use when you need to run Uvicorn within an already running asynchronous environment (e.g., an existing asyncio event loop) [4][5]. Usage guidelines for these methods remain unchanged in version 0.46.0 [1][3]: - For standard applications, use uvicorn.run [4][5]. - For advanced control over the server lifecycle or when integrating with an existing event loop, manually instantiate uvicorn.Config and uvicorn.Server, then call await server.serve [4][5]. There are no source code changes in 0.46.0 that modify how the reload functionality interacts with these methods [1][3]. When using reload=True, it is standard practice to wrap the execution in an if name == "main": block to ensure proper process spawning [8].
Citations:
- 1: https://github.com/Kludex/uvicorn/releases/tag/0.46.0
- 2: https://github.com/Kludex/uvicorn/blob/master/docs/release-notes.md
- 3: https://uvicorn.dev/release-notes/
- 4: https://github.com/encode/uvicorn/blob/master/docs/index.md
- 5: https://uvicorn.dev/
- 6: Allow low-level use of Server Kludex/uvicorn#300
- 7: Eager Asynchronous Construction of App Interferes with Running Event Loop Kludex/uvicorn#941
- 8: https://uvicorn.dev/settings/?h=reload
🌐 Web query:
github encode uvicorn 0.46.0 uvicorn/main.py run server.serve reload supervisor
💡 Result:
In Uvicorn version 0.46.0, the server lifecycle and process management are handled primarily within uvicorn/main.py [1][2]. When you run the server, Uvicorn determines whether to spawn a supervisor based on the configuration [1][2]. Process Management and Supervisors: 1. Reload Supervisor (ChangeReload): If the --reload option is enabled, Uvicorn uses the ChangeReload supervisor [1][2][3]. This supervisor watches for file changes (typically using watchfiles) and restarts the server process when modifications are detected [4][5][6]. 2. Multiprocess Supervisor (Multiprocess): If the --workers option is set to a value greater than 1, Uvicorn uses the Multiprocess supervisor [1][2][3]. This supervisor manages multiple worker processes to handle requests in parallel and provides health monitoring, restarting workers if they die unexpectedly [3][7]. 3. Execution: Both the --reload and --workers arguments are mutually exclusive; you cannot use them simultaneously [4][7][6]. If neither is required, the server runs directly via server.run [1][2]. Server Lifecycle (server.serve): The uvicorn.Server.serve method is an asynchronous entry point intended for environments where an event loop is already running [8]. When running from the command line or via uvicorn.run, the server lifecycle is managed by the main entry point, which handles socket binding (using config.bind_socket) before passing control to the appropriate supervisor or running the server directly [1][2]. Important Note: When running Uvicorn programmatically with reload=True or multiple workers, you must place the uvicorn.run call inside an if name == "main": block to avoid spawning recursive processes [4][9]. For external process managers like Supervisord, Uvicorn can be integrated by either handing over a file descriptor (fd) or using a UNIX domain socket [7]. It is generally recommended to use such external managers for production deployments, while the built-in reload functionality is intended for local development [3][7].
Citations:
- 1: https://github.com/encode/uvicorn/blob/master/uvicorn/main.py
- 2: https://github.com/encode/uvicorn/blob/6757386460735594b98d6b75e1df6974169909cf/uvicorn/main.py
- 3: https://deepwiki.com/encode/uvicorn/4.2-process-management
- 4: https://github.com/encode/uvicorn/blob/master/docs/settings.md
- 5: https://github.com/encode/uvicorn/blob/master/uvicorn/supervisors/basereload.py
- 6: https://uvicorn.dev/settings/
- 7: https://uvicorn.dev/deployment/
- 8: https://github.com/encode/uvicorn/blob/master/docs/index.md
- 9: https://uvicorn.dev/settings/?h=reload
🌐 Web query:
github encode uvicorn 0.46.0 uvicorn/main.py ChangeReload Supervisor
💡 Result:
In Uvicorn version 0.46.0, ChangeReload remains a supervisor class defined and used within uvicorn/main.py [1][2][3]. It is responsible for handling the automatic server reloading functionality when file changes are detected [4][5]. In the uvicorn/main.py file, the ChangeReload class is imported from uvicorn.supervisors [1][2][3]. The application logic checks the configuration to determine if reloading is enabled (via config.should_reload) [1][2]. If enabled, Uvicorn binds a socket and instantiates ChangeReload, passing the configuration and the server's run method as the target, before calling its run method to begin supervision [1][2][3]. Note that ChangeReload is generally considered an internal, private class, and its API or implementation details may change between versions [4]. While developers have occasionally extended or interacted with it for custom workflows (such as injecting linting checks before a reload), this usage is not officially supported and carries a risk of breaking with future updates [4]. The release notes for version 0.46.0 (released April 23, 2026) focus on WebSocket support and internal buffer changes, with no breaking changes to the supervisor logic noted [6][7][8].
Citations:
- 1: https://github.com/encode/uvicorn/blob/master/uvicorn/main.py
- 2: https://github.com/encode/uvicorn/blob/5bf788f0eb0fc771f5c4eed8f282c7ec256565d2/uvicorn/main.py
- 3: https://github.com/encode/uvicorn/blob/2351d5ff2b068d7bcba6eaccde83bc90b0727c89/uvicorn/main.py
- 4: https://jtmiclat.me/posts/adding-linting-as-part-of-python-server-reload/
- 5: Reload options doesn't work when you use uvicorn.Server(config).serve() Kludex/uvicorn#1868
- 6: https://github.com/Kludex/uvicorn/blob/main/docs/release-notes.md
- 7: https://github.com/Kludex/uvicorn/blob/master/docs/release-notes.md
- 8: https://uvicorn.dev/release-notes/
🌐 Web query:
raw.githubusercontent.com encode uvicorn master uvicorn/main.py def run 0.46.0
💡 Result:
In Uvicorn version 0.46.0, the run function is located in uvicorn/main.py [1]. It serves as the programmatic interface for starting a Uvicorn server, equivalent to the command line interface [2][3]. The signature of the run function in the Uvicorn source code includes numerous configuration parameters, such as host, port, loop, http, ws, reload, and workers, among others [1]. It handles the configuration of the ASGI application, server initialization, and process management (such as managing reloads or worker processes) based on the provided arguments [1]. The function is typically used in a Python script as follows: import uvicorn if name == "main": uvicorn.run("main:app", port=5000, log_level="info") When using features like reload=True or multiple workers, it is recommended to wrap the run call within an if name == 'main': block to ensure proper functionality [4]. Uvicorn 0.46.0 was released on April 23, 2026 [5].
Citations:
- 1: https://github.com/encode/uvicorn/blob/master/uvicorn/main.py
- 2: https://github.com/encode/uvicorn/blob/master/docs/index.md
- 3: https://uvicorn.dev/?featured_on=talkpython
- 4: https://uvicorn.dev/settings/
- 5: https://uvicorn.dev/release-notes/
🌐 Web query:
github.com encode uvicorn main.py run config.should_reload ChangeReload server.run server.serve
💡 Result:
In Uvicorn, the interaction between config.should_reload, ChangeReload, server.run, and server.serve is central to how the application handles automatic reloading during development [1][2]. 1. config.should_reload: This is a boolean property in the Uvicorn Config object [3][4]. It determines whether the server should operate in reload mode, which is triggered when the --reload flag is used or when specific reload-related options are configured [3][4]. Note that Uvicorn typically requires the application to be passed as an import string (rather than an object) for automatic reloading to work, as the supervisor needs to know how to re-import the application process [1][2][5]. 2. ChangeReload: This is a supervisor class found in uvicorn.supervisors [1][2]. When config.should_reload is true, the main entry point (often seen in uvicorn/main.py) instantiates ChangeReload [1][2]. It takes the server.run method as a target and a list of sockets [1][2]. Its responsibility is to monitor file changes, shut down the server process, and restart it by calling the provided target (server.run) [1][6]. 3. server.run vs. server.serve: - server.run: This is a synchronous method used to start the server [1][2]. It is the standard target used by supervisors like ChangeReload and Multiprocess to manage the server lifecycle [1][2]. - server.serve: This is an asynchronous method used to start the server [7]. Crucially, ChangeReload and automatic reloading are generally not integrated with serve [7]. If you instantiate and use uvicorn.Server(config).serve directly, the reload configuration is typically ignored [7]. In summary, when you run Uvicorn, the main script checks config.should_reload [1][2]. If enabled, it bypasses calling server.run directly and instead delegates control to ChangeReload, which manages the lifecycle of the server process by invoking server.run as needed [1][2][8]. Users are generally discouraged from extending or relying on ChangeReload directly, as it is considered a private API and its implementation is subject to change [6].
Citations:
- 1: https://github.com/encode/uvicorn/blob/master/uvicorn/main.py
- 2: https://github.com/encode/uvicorn/blob/2351d5ff2b068d7bcba6eaccde83bc90b0727c89/uvicorn/main.py
- 3: https://github.com/Kludex/uvicorn/blob/main/uvicorn/config.py
- 4: https://github.com/encode/uvicorn/blob/master/uvicorn/config.py
- 5: Reload only working when app is supplied as string Kludex/uvicorn#2553
- 6: https://jtmiclat.me/posts/adding-linting-as-part-of-python-server-reload/
- 7: Reload options doesn't work when you use uvicorn.Server(config).serve() Kludex/uvicorn#1868
- 8: Make force_exit configure-able from command line Kludex/uvicorn#675
Maintain Uvicorn’s reload supervisor in the Windows path.
This branch still calls Server.serve() directly when reload=True; on Uvicorn >= 0.46.0, reload is driven through the main entry point supervisory path, so Windows development startup can silently bypass file-change restarts. Keep the reload supervisor in this path for reload=True, or reject reload=True on Windows explicitly.
🤖 Prompt for 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.
In `@run.py` around lines 95 - 104, The Windows branch in the startup flow must
preserve Uvicorn’s reload supervision. Update the logic around the
Windows-specific SelectorEventLoop and Server.serve call so reload=True uses
Uvicorn’s main entry-point path, or explicitly reject reload=True on Windows;
retain the existing direct server behavior only when reload is disabled.
…ad, bg test - Annotate _flush_context parameter with object type - Move _schedule_async_image_verification after explicit commit - Preserve uvicorn reload supervision on Windows (use uvicorn.run when reload=True) - Add get_bg_db flushed-commit regression test
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/services/user.py">
<violation number="1" location="app/services/user.py:1019">
P2: A refresh failure after this commit returns a 500 even though the profile update is already durable; dependency rollback cannot restore atomicity. Refresh before committing so refresh/query failures still roll back the pending update.</violation>
</file>
<file name="app/core/database.py">
<violation number="1" location="app/core/database.py:294">
P1: Writes made before a successful or failed savepoint can be rolled back at request teardown because nested `begin_nested()` completion clears the outer transaction's commit marker. Preserve the marker until the root transaction ends; apply the same distinction to the rollback handler.</violation>
<violation number="2" location="app/core/database.py:302">
P1: Direct SQLAlchemy DML is still not detected by the new commit tracker. `AsyncSession.execute(insert/update/delete(...))` does not populate `new/dirty/deleted` or invoke `after_flush`, so a request such as the `UserSwipe` upsert in `app/services/swipe.py` can return successfully and then be rolled back when `get_db` skips `commit()`. The tracker could also mark writes from `after_execute`/an explicit mutation helper, or all direct-DML callers should commit explicitly.</violation>
</file>
<file name="run.py">
<violation number="1" location="run.py:101">
P2: Windows development no longer hot-reloads: this branch passes `reload=True` into `Config` but calls `Server.serve()` directly, bypassing Uvicorn's `ChangeReload` supervisor. Keeping the existing `uvicorn.run("app.main:app", **uvicorn_kwargs)` path after setting the selector policy would retain auto-reload while using the configured Windows loop policy.</violation>
</file>
<file name="app/main.py">
<violation number="1" location="app/main.py:7">
P2: The new Windows policy block is placed between the standard-library and third-party imports, so Ruff's enforced E402 rule treats the remaining imports as late imports and the lint job fails. Moving the policy initialization below the complete import block preserves the behavior without violating the repository's import contract.</violation>
<violation number="2" location="app/main.py:8">
P2: The Windows selector policy in `app/main.py` does not force a selector loop for direct Uvicorn/FastAPI-CLI launches: Uvicorn 0.46.0 explicitly creates a `ProactorEventLoop` before importing the application. If those entrypoints need the same Windows compatibility fix, the loop must be selected in the launcher/Uvicorn configuration rather than only during application import.</violation>
</file>
<file name="app/services/property/crud.py">
<violation number="1" location="app/services/property/crud.py:284">
P2: Broken image cleanup can be lost because the verification task is started before the property transaction commits. The fresh session used by `_schedule_async_image_verification()` may query/update before this `await db.commit()`, see no uncommitted rows, and then the later commit persists the bad image URLs unchanged. Scheduling the task after a successful commit (or awaiting a post-commit hook) would preserve the documented safety-net behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| @event.listens_for(Session, "after_commit") | ||
| def _clear_session_needs_commit_on_commit(session: Session) -> None: | ||
| session.info.pop(_SESSION_NEEDS_COMMIT_KEY, None) |
There was a problem hiding this comment.
P1: Writes made before a successful or failed savepoint can be rolled back at request teardown because nested begin_nested() completion clears the outer transaction's commit marker. Preserve the marker until the root transaction ends; apply the same distinction to the rollback handler.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/core/database.py, line 294:
<comment>Writes made before a successful or failed savepoint can be rolled back at request teardown because nested `begin_nested()` completion clears the outer transaction's commit marker. Preserve the marker until the root transaction ends; apply the same distinction to the rollback handler.</comment>
<file context>
@@ -276,6 +276,37 @@ def _on_checkin(dbapi_conn, connection_record):
+
+@event.listens_for(Session, "after_commit")
+def _clear_session_needs_commit_on_commit(session: Session) -> None:
+ session.info.pop(_SESSION_NEEDS_COMMIT_KEY, None)
+
+
</file context>
| session.info.pop(_SESSION_NEEDS_COMMIT_KEY, None) | ||
|
|
||
|
|
||
| def _session_needs_commit(session: AsyncSession | Session) -> bool: |
There was a problem hiding this comment.
P1: Direct SQLAlchemy DML is still not detected by the new commit tracker. AsyncSession.execute(insert/update/delete(...)) does not populate new/dirty/deleted or invoke after_flush, so a request such as the UserSwipe upsert in app/services/swipe.py can return successfully and then be rolled back when get_db skips commit(). The tracker could also mark writes from after_execute/an explicit mutation helper, or all direct-DML callers should commit explicitly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/core/database.py, line 302:
<comment>Direct SQLAlchemy DML is still not detected by the new commit tracker. `AsyncSession.execute(insert/update/delete(...))` does not populate `new/dirty/deleted` or invoke `after_flush`, so a request such as the `UserSwipe` upsert in `app/services/swipe.py` can return successfully and then be rolled back when `get_db` skips `commit()`. The tracker could also mark writes from `after_execute`/an explicit mutation helper, or all direct-DML callers should commit explicitly.</comment>
<file context>
@@ -276,6 +276,37 @@ def _on_checkin(dbapi_conn, connection_record):
+ session.info.pop(_SESSION_NEEDS_COMMIT_KEY, None)
+
+
+def _session_needs_commit(session: AsyncSession | Session) -> bool:
+ """True when this request mutated DB state that still needs a commit."""
+ sync = getattr(session, "sync_session", session)
</file context>
| # cleanup *after* the response is sent, so a follow-up | ||
| # GET /users/me/auth-state from the client can race an uncommitted | ||
| # flush and still report profile_completion (missing full_name/DOB). | ||
| await db.commit() |
There was a problem hiding this comment.
P2: A refresh failure after this commit returns a 500 even though the profile update is already durable; dependency rollback cannot restore atomicity. Refresh before committing so refresh/query failures still roll back the pending update.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/services/user.py, line 1019:
<comment>A refresh failure after this commit returns a 500 even though the profile update is already durable; dependency rollback cannot restore atomicity. Refresh before committing so refresh/query failures still roll back the pending update.</comment>
<file context>
@@ -986,9 +1007,16 @@ async def update_user(
+ # cleanup *after* the response is sent, so a follow-up
+ # GET /users/me/auth-state from the client can race an uncommitted
+ # flush and still report profile_completion (missing full_name/DOB).
+ await db.commit()
await db.refresh(user)
logger.info("User %s updated successfully", user_id)
</file context>
| loop = asyncio.SelectorEventLoop(selector) | ||
| asyncio.set_event_loop(loop) | ||
| config = uvicorn.Config("app.main:app", **uvicorn_kwargs) | ||
| server = uvicorn.Server(config) |
There was a problem hiding this comment.
P2: Windows development no longer hot-reloads: this branch passes reload=True into Config but calls Server.serve() directly, bypassing Uvicorn's ChangeReload supervisor. Keeping the existing uvicorn.run("app.main:app", **uvicorn_kwargs) path after setting the selector policy would retain auto-reload while using the configured Windows loop policy.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At run.py, line 101:
<comment>Windows development no longer hot-reloads: this branch passes `reload=True` into `Config` but calls `Server.serve()` directly, bypassing Uvicorn's `ChangeReload` supervisor. Keeping the existing `uvicorn.run("app.main:app", **uvicorn_kwargs)` path after setting the selector policy would retain auto-reload while using the configured Windows loop policy.</comment>
<file context>
@@ -88,5 +92,13 @@
+ loop = asyncio.SelectorEventLoop(selector)
+ asyncio.set_event_loop(loop)
+ config = uvicorn.Config("app.main:app", **uvicorn_kwargs)
+ server = uvicorn.Server(config)
+ loop.run_until_complete(server.serve())
+ else:
</file context>
| import sys | ||
|
|
||
| if sys.platform == "win32": | ||
| asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy()) |
There was a problem hiding this comment.
P2: The Windows selector policy in app/main.py does not force a selector loop for direct Uvicorn/FastAPI-CLI launches: Uvicorn 0.46.0 explicitly creates a ProactorEventLoop before importing the application. If those entrypoints need the same Windows compatibility fix, the loop must be selected in the launcher/Uvicorn configuration rather than only during application import.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/main.py, line 8:
<comment>The Windows selector policy in `app/main.py` does not force a selector loop for direct Uvicorn/FastAPI-CLI launches: Uvicorn 0.46.0 explicitly creates a `ProactorEventLoop` before importing the application. If those entrypoints need the same Windows compatibility fix, the loop must be selected in the launcher/Uvicorn configuration rather than only during application import.</comment>
<file context>
@@ -2,6 +2,10 @@
+import sys
+
+if sys.platform == "win32":
+ asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy())
import sentry_sdk
</file context>
| import logging | ||
| import sys | ||
|
|
||
| if sys.platform == "win32": |
There was a problem hiding this comment.
P2: The new Windows policy block is placed between the standard-library and third-party imports, so Ruff's enforced E402 rule treats the remaining imports as late imports and the lint job fails. Moving the policy initialization below the complete import block preserves the behavior without violating the repository's import contract.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/main.py, line 7:
<comment>The new Windows policy block is placed between the standard-library and third-party imports, so Ruff's enforced E402 rule treats the remaining imports as late imports and the lint job fails. Moving the policy initialization below the complete import block preserves the behavior without violating the repository's import contract.</comment>
<file context>
@@ -2,6 +2,10 @@
import logging
+import sys
+
+if sys.platform == "win32":
+ asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy())
</file context>
| # Flatmate path flushes prescreen metadata which clears UoW collections; | ||
| # without a commit here (and the needs_commit session flag in get_db), | ||
| # the request returned 200 then rolled back — listings vanished on reload. | ||
| await db.commit() |
There was a problem hiding this comment.
P2: Broken image cleanup can be lost because the verification task is started before the property transaction commits. The fresh session used by _schedule_async_image_verification() may query/update before this await db.commit(), see no uncommitted rows, and then the later commit persists the bad image URLs unchanged. Scheduling the task after a successful commit (or awaiting a post-commit hook) would preserve the documented safety-net behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/services/property/crud.py, line 289:
<comment>Broken image cleanup can be lost because the verification task is started before the property transaction commits. The fresh session used by `_schedule_async_image_verification()` may query/update before this `await db.commit()`, see no uncommitted rows, and then the later commit persists the bad image URLs unchanged. Scheduling the task after a successful commit (or awaiting a post-commit hook) would preserve the documented safety-net behavior.</comment>
<file context>
@@ -282,6 +282,12 @@ async def create_property(
+ # Flatmate path flushes prescreen metadata which clears UoW collections;
+ # without a commit here (and the needs_commit session flag in get_db),
+ # the request returned 200 then rolled back — listings vanished on reload.
+ await db.commit()
+
logger.info("Property created successfully with ID %s", db_property.id)
</file context>
There was a problem hiding this comment.
0 issues found across 4 files (changes from recent commits).
Requires human review: Auto-approval blocked by 7 unresolved issues from previous reviews.
Re-trigger cubic
2 participants
Changes
Session commit tracking (fixes flatmate listings vanishing)
needs_commitsession flag via SQLAlchemyafter_flushevent listenerget_dbandget_bg_dbnow use_session_needs_commit()instead of only checking UoW collectionsflush()clearedsession.new/dirty/deletedExplicit commits
create_property: explicitawait db.commit()after prescreen flushupdate_user: explicitawait db.commit()before response to prevent auth-state raceDOB coercion
_coerce_date_of_birth_for_storage(): convertsdateto UTCdatetimefor theDateTime(timezone=True)ORM columnWindows compatibility
WindowsSelectorEventLoopPolicyon win32 inrun.pyandapp/main.pySelectorEventLoopexplicitly for uvicorn on WindowsTests
test_get_db_commits_after_flush_even_when_uow_looks_cleantest_self_update_coerces_date_of_birth_to_utc_datetimeSummary by CodeRabbit
Bug Fixes
Compatibility