refactor(api): use sessionmaker in end user, retention & cleanup services#34765
Conversation
Pyrefly Diffbase → PR--- /tmp/pyrefly_base.txt 2026-04-08 19:22:13.055706706 +0000
+++ /tmp/pyrefly_pr.txt 2026-04-08 19:22:04.515686160 +0000
@@ -6399,9 +6399,9 @@
ERROR Argument `FakeRepo` is not assignable to parameter `workflow_run_repo` with type `APIWorkflowRunRepository | None` in function `services.retention.workflow_run.clear_free_plan_expired_workflow_run_logs.WorkflowRunCleanup.__init__` [bad-argument-type]
--> tests/unit_tests/services/test_clear_free_plan_expired_workflow_run_logs.py:114:49
ERROR Class member `FixedDateTime.now` overrides parent class `datetime` in an inconsistent manner [bad-override]
- --> tests/unit_tests/services/test_clear_free_plan_tenant_expired_logs.py:411:13
+ --> tests/unit_tests/services/test_clear_free_plan_tenant_expired_logs.py:407:13
ERROR Class member `FixedDateTime.now` overrides parent class `datetime` in an inconsistent manner [bad-override]
- --> tests/unit_tests/services/test_clear_free_plan_tenant_expired_logs.py:505:13
+ --> tests/unit_tests/services/test_clear_free_plan_tenant_expired_logs.py:501:13
ERROR Argument `SimpleNamespace` is not assignable to parameter `account` with type `Account` in function `services.dataset_service.DatasetService.create_empty_dataset` [bad-argument-type]
--> tests/unit_tests/services/test_dataset_service_dataset.py:310:93
ERROR Argument `SimpleNamespace` is not assignable to parameter `account` with type `Account` in function `services.dataset_service.DatasetService.create_empty_dataset` [bad-argument-type]
|
There was a problem hiding this comment.
Pull request overview
Refactors several API services to use sessionmaker(...).begin() for transaction management instead of manually creating Session(...) objects and calling session.commit(), and updates unit tests to mock the new pattern.
Changes:
- Migrate EndUser and retention message cleanup flows to
sessionmaker(...).begin()transaction contexts. - Replace
Session(...).no_autoflushusage in the free-plan log cleanup flow withsessionmaker(..., autoflush=False).begin(). - Update unit-test session mocks to emulate
sessionmaker().begin()semantics.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| api/services/end_user_service.py | Switches EndUser DB interactions to sessionmaker(...).begin() and removes explicit commits. |
| api/services/retention/conversation/messages_clean_service.py | Uses sessionmaker(...).begin() for fetch/delete transactions and adjusts commit timing logic. |
| api/services/clear_free_plan_tenant_expired_logs.py | Replaces no_autoflush session usage with sessionmaker(..., autoflush=False).begin() and removes explicit commits. |
| api/tests/unit_tests/services/test_clear_free_plan_tenant_expired_logs.py | Updates monkeypatch/mocks to match the sessionmaker().begin() pattern. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| delete_result = cast(CursorResult, session.execute(delete_stmt)) | ||
| messages_deleted = delete_result.rowcount | ||
| delete_messages_ms = int((time.monotonic() - delete_messages_start) * 1000) | ||
| commit_start = time.monotonic() | ||
| session.commit() | ||
| commit_ms = int((time.monotonic() - commit_start) * 1000) | ||
| commit_ms = 0 |
There was a problem hiding this comment.
commit_ms is now hard-coded to 0, but the transaction commit actually happens when the sessionmaker(...).begin() context exits. This makes the timing log misleading. Consider either (a) removing the commit timing from the log, or (b) restructuring to explicitly time the transaction end (e.g., measure around the context manager exit, or switch to a with sessionmaker(... )() as session: + explicit session.commit() if you need accurate commit timing).
Summary
with Session(db.engine, expire_on_commit=False)+session.commit()withsessionmaker(bind=db.engine, expire_on_commit=False).begin()in end_user_service and messages_clean_servicewith Session(db.engine).no_autoflush+session.commit()withsessionmaker(bind=db.engine, autoflush=False).begin()in clear_free_plan_tenant_expired_logsPart of #24245
Test plan
ruff checkpasses