Refactor: session-per-interaction — DB sessions never survive network calls#1
Open
Vector897 wants to merge 1 commit into
Open
Refactor: session-per-interaction — DB sessions never survive network calls#1Vector897 wants to merge 1 commit into
Vector897 wants to merge 1 commit into
Conversation
…work call Root-cause fix for the site-wide POST deadlocks. The old convention threaded one long-lived session through engine -> graphs -> llm -> budget/cache/audit -> notify, with LLM calls, PDF downloads and Telegram/SMTP pushes happening inside the open transaction. Under SQLite's single-writer model any dirty state autoflushed by an in-step SELECT armed a write lock that was then held across up-to-120s network waits, blocking every other writer. Previous commits patched individual sites; this makes the class of bug impossible. New convention (enforced across worker/scheduler paths): - engine.run_task(task_id) owns no session; every bookkeeping write (status/progress/checkpoint/duration) is its own short committed session - TaskContext.db is gone; steps use `with ctx.session() as db:` scoped to DB-only blocks; fetch/download/LLM run sessionless - llm.complete(prompt, task_id=...) is the exemplar: (1) short session for redact/cache/budget and a full routing plan incl. decrypted keys, (2) pure-network litellm call, (3) short session for audit/cache/cost - cascade, notify_all, memory.consolidate/_arbitrate follow the same shape; notify and consolidate now manage their own sessions - runner passes task_id only; nightly consolidation no longer holds a session across per-user LLM calls Tests: updated to new signatures; added a regression guard that runs a task whose step waits 2s "on the network" and asserts a concurrent writer commits in <1s. 15/15 pass. Co-Authored-By: Claude Opus 4.8 <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR enforces a repo-wide “session-per-interaction” discipline to prevent SQLite write locks from being held across network I/O (LLM calls, PDF downloads, Telegram/SMTP), addressing the deadlock class that previously stalled site-wide POSTs.
Changes:
- Refactors the task engine and step APIs so workers pass
task_idonly and all DB writes/reads occur in short-lived sessions. - Refactors LLM routing/cascade, memory consolidation/arbitration, and notification sending to self-manage short sessions and keep network calls sessionless.
- Updates/extends tests, adding a concurrency regression guard to ensure concurrent writers aren’t blocked during step “network wait”.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/app/core/engine/engine.py | Refactors run_task/TaskContext to remove long-lived sessions and do bookkeeping via short sessions. |
| server/app/core/scheduler/runner.py | Updates worker + nightly consolidation loops to avoid holding sessions across per-task/per-user work. |
| server/app/core/router/llm.py | Splits LLM completion into (DB prep) → (network) → (DB audit) phases with short sessions. |
| server/app/core/router/cascade.py | Updates cascade calls to the new sessionless llm.complete(...) API (uses task_id). |
| server/app/core/memory/memory.py | Makes consolidate/arbitrate sessionless across LLM calls; uses short sessions for DB-only blocks. |
| server/app/core/engine/graphs/arxiv_watch.py | Moves DB operations into with ctx.session() and keeps PDF/LLM network work out of sessions. |
| server/app/core/engine/graphs/briefing.py | Applies the same pattern: DB gather/save in short sessions; LLM + notify outside sessions. |
| server/app/connectors/notify.py | Changes notify API to not accept a caller session; reads settings in a short session then sends network requests sessionless. |
| server/app/core/budget/guard.py | Adjusts budget-cutoff notification to call sessionless notify_all(...) after committing. |
| server/app/api/misc.py | Updates API endpoints to call the new notify_all(...) and cascade.complete_cascade(...) signatures. |
| server/tests/test_core.py | Updates engine tests for new signatures and adds a concurrency regression test. |
| server/tests/test_tier_features.py | Updates memory consolidation test to call sessionless consolidate(uid). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+152
to
+156
| steps = REGISTRY.get(task.type) | ||
| if not steps: | ||
| task.status = "FAILED" | ||
| task.error = f"Unknown task type: {task.type}" | ||
| return |
Comment on lines
+146
to
+166
| def slow_network(ctx, state): | ||
| _t.sleep(2.0) # 模拟 LLM/PDF 下载等网络等待（正确写法：不持有会话） | ||
| return state | ||
|
|
||
| register("_locktest", [StepDef("net", slow_network)]) | ||
| with session() as db: | ||
| user = User(name="lock_user", password_hash="x") | ||
| db.add(user) | ||
| db.flush() | ||
| task = Task(type="_locktest", owner_id=user.id, params_json="{}") | ||
| db.add(task) | ||
| db.flush() | ||
| task_id, owner_id = task.id, user.id | ||
|
|
||
| worker = threading.Thread(target=run_task, args=(task_id,)) | ||
| worker.start() | ||
| _t.sleep(0.5) # 等 worker 进入 slow_network 的等待段 | ||
| t0 = _t.time() | ||
| with session() as db: # 模拟 API 线程的 POST 写入 | ||
| db.add(Task(type="_locktest", owner_id=owner_id, params_json="{}", status="CANCELLED")) | ||
| blocked_for = _t.time() - t0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The site-wide POST deadlocks (news tasks stuck RUNNING, 0 artifacts, every submit hanging) all traced to one architectural convention: a single long-lived SQLAlchemy session threaded through engine → graphs → llm → budget/cache/audit → notify, with network I/O happening inside the open transaction. Under SQLite's single-writer model, any dirty object autoflushed by an in-step SELECT armed the write lock, which was then held across up-to-120s LLM calls / PDF downloads / Telegram+SMTP pushes — blocking every other writer in the process.
Commits e893129 / 548ec9f / d3c0af3 on main patched the individual sites and stabilized the demo. This PR removes the class of bug: with no session alive during any network call, there is no lock to hold.
New convention
run_task(task_id)owns no session. Every bookkeeping write (status, progress, checkpoint, step duration) opens a short session, commits, closes. Progress/artifacts are visible to API readers in real time as a side effect.ctx.dbis gone. Steps usewith ctx.session() as db:scoped to DB-only blocks; fetch/download/LLM run sessionless.task_idonly; nightly consolidation no longer holds a session across per-user LLM calls.Testing
Deploy note
No schema changes, no config changes. Same
git pull && docker compose build --no-cache && up -d. Recommended verification after deploy: submit 2–3 tasks while one is RUNNING (should queue instantly), check artifacts/audit appear per-step, run one briefing.🤖 Generated with Claude Code