Merged
Conversation
…and observer cleanup
…-deps, and update docs
…check to satisfy Ruff TRY300
…ifespan, and move asyncio import to stdlib group
…ion logging feedback
…nd update UI/README
….py fallbacks, and unused vars
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthrough채팅 경로에 성능 타이밍 계측(t0–t6), 전역 CHAT_TIMEOUT(30s) 및 DB RPC 동시성 제어 세마포어를 추가하고, 번역·임베딩·DB검색·제목생성 호출에 타임아웃·타이밍 로그·타임아웃 기반 에러 처리를 도입했습니다. llm 서비스의 스트림 종료 안전성 로직도 개선되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as Chat API
participant Translator as Translation Service
participant Embed as Embedding Service
participant DB as Database (Supabase)
participant LLM as LLM Service / Stream
rect rgba(200,220,255,0.5)
Client->>API: 요청 (채팅 메시지)
API->>Translator: 번역 (async, timeout=30s)
Translator-->>API: 번역된 텍스트 + elapsed
end
rect rgba(200,255,200,0.5)
API->>Embed: 임베딩 생성 (async, timeout=30s)
Embed-->>API: 벡터 + elapsed
end
rect rgba(255,230,200,0.5)
API->>DB: 문서 검색 (세마포어 획득, timeout=30s)
DB-->>API: 문서 목록 + elapsed
end
rect rgba(255,200,220,0.5)
API->>LLM: 스트리밍 생성 (stream)
LLM-->>API: 첫 청크 (till first chunk time logged)
LLM-->>Client: SSE 청크 스트리밍
Client-->>API: 연결 끊김 (optional)
API->>API: 스트리밍 완료 로그 (청크수/총시간) 또는 폴백 전송
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
개요두 개의 백엔드 파일에서 성능 타이밍 계측을 추가하고, 번역, 임베딩, 데이터베이스 검색 및 채팅 제목 생성의 타임아웃을 10초에서 30초로 증가시켰으며, 경과 시간 정보가 포함된 개선된 에러 처리 및 로깅을 구현했습니다. 변경사항
추정 코드 리뷰 노력🎯 4 (Complex) | ⏱️ ~45 minutes 관련 가능성이 있는 PR
시
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/api/routes/chat.py`:
- Around line 82-87: The DB RPC call using _search_documents must have an
explicit timeout and null-safe handling: wrap the
asyncio.to_thread(_search_documents, query_vector) call with asyncio.wait_for
using a sensible timeout, catch asyncio.TimeoutError and log via logger.error,
and on success ensure response.data is treated safely (e.g., set documents =
response.data or [] if response.data is None) before calling len(documents) in
logger.info; update the try/except to handle both timeout and generic exceptions
and ensure documents is always a list when referenced.
In `@backend/app/services/llm.py`:
- Around line 135-138: The warning log currently prints the raw user query
(logger.warning(... Query: {query})), which exposes PII; change the
logger.warning call in backend/app/services/llm.py to omit the user text and
instead log only non-sensitive metadata (for example the query length and a
request ID or correlation_id if available, or generate/attach one like
request_id), e.g., include variables such as len(query) and
request_id/correlation_id; leave the subsequent raise as-is. Ensure you
reference and use the existing logger variable and the query variable only to
compute length (not the content).
- Around line 139-140: finally 블록에서 await generator.aclose()가 실행될 때 발생한 예외가 이전에
발생한 TimeoutError 같은 원래 예외를 덮어쓸 수 있으므로, generator.aclose() 호출을 별도의 보호문으로 감싸 원래
예외가 유지되도록 처리하세요: 즉 현재 finally 블록 안의 await generator.aclose()를 try/except로
감싸고(aclose 관련 예외는 로그만 남기거나 무시) 원래 예외가 있으면 재발생시키거나 그대로 유지되도록 변경하여
generator.aclose() 예외가 원래 TimeoutError을 덮지 않게 하세요 (대상 식별자: generator.aclose(),
finally 블록, TimeoutError).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a014b00a-6eba-4574-9fbb-45ca038296d3
📒 Files selected for processing (6)
backend/app/api/routes/chat.pybackend/app/core/config.pybackend/app/main.pybackend/app/services/embedding.pybackend/app/services/llm.pybackend/requirements.txt
…null-safe, logging)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/app/api/routes/chat.py (1)
48-49: 타임아웃 값(30.0)은 상수로 올려서 드리프트를 막는 것을 권장합니다.현재 여러 지점에 하드코딩되어 있어 추후 조정 시 누락 위험이 있습니다.
Also applies to: 66-67, 85-86, 199-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/chat.py` around lines 48 - 49, Multiple calls set timeout=30.0 inline; extract this magic value to a module-level constant (e.g., CHAT_TIMEOUT = 30.0) and replace all occurrences of timeout=30.0 with timeout=CHAT_TIMEOUT to prevent drift and make future adjustments safe; ensure the constant is defined near the top of the module and update any imports/uses in the same module so functions/methods that currently pass timeout=30.0 (look for the literal timeout=30.0 instances) reference CHAT_TIMEOUT instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/api/routes/chat.py`:
- Around line 83-86: The Supabase client is created without RPC/HTTP timeouts so
asyncio.wait_for(asyncio.to_thread(_search_documents, ...)) only cancels the
wrapper thread but doesn't stop the underlying DB RPC; update get_client() in
backend/app/services/database.py to call create_client(supabase_url,
supabase_key, ClientOptions(postgrest_client_timeout=30)) (or equivalent
ClientOptions param) so the HTTP/RPC call has a 30s timeout, and in
backend/app/api/routes/chat.py wrap calls to
asyncio.wait_for(asyncio.to_thread(_search_documents, query_vector), ...) with a
semaphore to limit concurrent executions (create/await an asyncio.Semaphore and
acquire/release around the wait_for/to_thread block) to prevent thread-pool
exhaustion.
---
Nitpick comments:
In `@backend/app/api/routes/chat.py`:
- Around line 48-49: Multiple calls set timeout=30.0 inline; extract this magic
value to a module-level constant (e.g., CHAT_TIMEOUT = 30.0) and replace all
occurrences of timeout=30.0 with timeout=CHAT_TIMEOUT to prevent drift and make
future adjustments safe; ensure the constant is defined near the top of the
module and update any imports/uses in the same module so functions/methods that
currently pass timeout=30.0 (look for the literal timeout=30.0 instances)
reference CHAT_TIMEOUT instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 525d0865-5d2a-4d31-b8d6-f8a804656cac
📒 Files selected for processing (2)
backend/app/api/routes/chat.pybackend/app/services/llm.py
Comment on lines
+83
to
+86
| response = await asyncio.wait_for( | ||
| asyncio.to_thread(_search_documents, query_vector), | ||
| timeout=30.0, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find database.py file
fd database.py
# Check its location and size
find . -name database.py -type f -exec wc -l {} \;Repository: SanghunYun95/philo-rag
Length of output: 136
🏁 Script executed:
#!/bin/bash
# Search for timeout configurations in database.py
rg -n -C3 'create_client|ClientOptions|timeout|postgrest|httpx|supabase' backend/app/services/database.py 2>/dev/null || echo "File not found at expected location"
# Also try to find database.py in other locations
fd database.py -x cat {} \;Repository: SanghunYun95/philo-rag
Length of output: 2228
🏁 Script executed:
#!/bin/bash
# Verify the wait_for(to_thread pattern in chat.py
rg -n -C5 'asyncio\.wait_for\(\s*asyncio\.to_thread\(' backend/app/api/routes/chat.pyRepository: SanghunYun95/philo-rag
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Find and examine _search_documents function
rg -n -B2 -A10 'def _search_documents' backend/Repository: SanghunYun95/philo-rag
Length of output: 939
🏁 Script executed:
#!/bin/bash
# Read chat.py lines 80-95 to see the exact code at lines 83-86
sed -n '75,95p' backend/app/api/routes/chat.pyRepository: SanghunYun95/philo-rag
Length of output: 985
🏁 Script executed:
#!/bin/bash
# Check entire chat.py to understand the context
wc -l backend/app/api/routes/chat.py
cat -n backend/app/api/routes/chat.pyRepository: SanghunYun95/philo-rag
Length of output: 9740
🏁 Script executed:
#!/bin/bash
# Search for any timeout configuration in the entire codebase
rg -n 'timeout' backend/ --type pyRepository: SanghunYun95/philo-rag
Length of output: 1141
🏁 Script executed:
#!/bin/bash
# Verify Supabase Python client supports timeout configuration
rg -n 'from supabase|import supabase' backend/ --type py
# Check if there's any ClientOptions or timeout configuration attempts
rg -n 'ClientOptions|timeout' backend/app/services/ --type pyRepository: SanghunYun95/philo-rag
Length of output: 674
🌐 Web query:
Supabase Python SDK httpx timeout configuration create_client
💡 Result:
In supabase-py, create_client(url, key, options=...) expects a ClientOptions object, not a plain dict. Timeouts are configured via ClientOptions fields (and/or by supplying your own httpx.Client). [1]
Sync client (create_client)
import httpx
from supabase import create_client, ClientOptions
# Option A: set the PostgREST (DB REST API) timeout only
opts = ClientOptions(postgrest_client_timeout=30) # seconds (or httpx.Timeout)
supabase = create_client(SUPABASE_URL, SUPABASE_KEY, options=opts)postgrest_client_timeout is passed into the underlying PostgREST client as its request timeout. [1][2]
import httpx
from supabase import create_client, ClientOptions
# Option B: supply a custom httpx client (shared by PostgREST, Auth, Storage, Functions)
http = httpx.Client(timeout=httpx.Timeout(30.0, connect=5.0))
opts = ClientOptions(httpx_client=http)
supabase = create_client(SUPABASE_URL, SUPABASE_KEY, options=opts)If httpx_client is provided, Supabase uses it for PostgREST/Auth/Storage/Functions. [1][2]
Async client (create_async_client / acreate_client)
Use AsyncClientOptions(httpx_client=...) with an httpx.AsyncClient. [1]
Other per-service timeouts available on ClientOptions:
storage_client_timeoutfunction_client_timeoutpostgrest_client_timeout[1]
Sources:
[1] ClientOptions / AsyncClientOptions definition (timeouts + httpx_client) (raw.githubusercontent.com)
[2] Sync client wiring: how postgrest_client_timeout / httpx_client are applied (raw.githubusercontent.com)
Citations:
- 1: https://raw.githubusercontent.com/supabase/supabase-py/main/src/supabase/src/supabase/lib/client_options.py
- 2: https://raw.githubusercontent.com/supabase/supabase-py/main/src/supabase/src/supabase/_sync/client.py
wait_for(to_thread(...)) 타임아웃은 DB RPC 작업을 실제로 중단하지 못합니다.
backend/app/services/database.py의 get_client()에서 Supabase 클라이언트를 생성할 때 ClientOptions를 통한 HTTP/RPC 타임아웃이 설정되지 않았습니다. 현재 코드는 create_client(supabase_url, supabase_key)만 호출하므로, RPC 요청은 무한정 대기할 수 있습니다. 이 상태에서 asyncio.wait_for(asyncio.to_thread(...))는 asyncio 래퍼만 취소하고 스레드의 실제 RPC 호출을 중단하지 못하므로, 타임아웃 발생 후에도 _search_documents 스레드 작업이 계속 실행되어 스레드 풀 고갈로 이어질 수 있습니다.
필수 조치:
backend/app/services/database.py에서 클라이언트 생성 시ClientOptions(postgrest_client_timeout=30)을 전달- 추가적으로 동시성 제한(semaphore)을
wait_for블록에 적용
해결 방법
import threading
from supabase import create_client, Client, ClientOptions
from app.core.config import settings
SUPABASE_CONFIG_ERROR = "SUPABASE_URL and SUPABASE_SERVICE_KEY must be configured"
def _get_supabase_client() -> Client:
supabase_url = settings.SUPABASE_URL
supabase_key = settings.SUPABASE_SERVICE_KEY
if not supabase_url or not supabase_key:
raise RuntimeError(SUPABASE_CONFIG_ERROR)
+ options = ClientOptions(postgrest_client_timeout=30)
- return create_client(supabase_url, supabase_key)
+ return create_client(supabase_url, supabase_key, options=options)
_client_lock = threading.Lock()
_supabase_client: Client | None = None
def get_client() -> Client:
global _supabase_client
if _supabase_client is None:
with _client_lock:
if _supabase_client is None:
_supabase_client = _get_supabase_client()
return _supabase_client그리고 backend/app/api/routes/chat.py 라인 82–86에 동시성 제한 추가:
+_db_rpc_semaphore = asyncio.Semaphore(16)
try:
+ async with _db_rpc_semaphore:
response = await asyncio.wait_for(
asyncio.to_thread(_search_documents, query_vector),
timeout=30.0,
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/routes/chat.py` around lines 83 - 86, The Supabase client is
created without RPC/HTTP timeouts so
asyncio.wait_for(asyncio.to_thread(_search_documents, ...)) only cancels the
wrapper thread but doesn't stop the underlying DB RPC; update get_client() in
backend/app/services/database.py to call create_client(supabase_url,
supabase_key, ClientOptions(postgrest_client_timeout=30)) (or equivalent
ClientOptions param) so the HTTP/RPC call has a 30s timeout, and in
backend/app/api/routes/chat.py wrap calls to
asyncio.wait_for(asyncio.to_thread(_search_documents, query_vector), ...) with a
semaphore to limit concurrent executions (create/await an asyncio.Semaphore and
acquire/release around the wait_for/to_thread block) to prevent thread-pool
exhaustion.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/app/api/routes/chat.py (2)
17-20: 동시성 제한값도 설정화하면 운영 튜닝이 쉬워집니다.Line 20의
16은 환경별로 적정치가 달라질 수 있어 설정값으로 빼두는 것을 권장합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/chat.py` around lines 17 - 20, The semaphore concurrency hardcode (_db_rpc_semaphore = asyncio.Semaphore(16)) should be replaced with a configurable value; add or read a setting (e.g., DB_RPC_CONCURRENCY or a config entry) and use it to construct _db_rpc_semaphore (keeping CHAT_TIMEOUT as-is), validate/parse the env/config as int and provide a sensible default (16) if unset, and update any docstring or comment near _db_rpc_semaphore to note it is configurable.
157-164: 첫 청크 로그 시점이 실제 전송 시점과 어긋날 수 있습니다.Line 157의 로그가 disconnect 체크(Line 161)보다 먼저 실행되어, 실제 전송이 없는데도 첫 청크 수신 로그가 찍힐 수 있습니다.
🧹 제안 수정
async for chunk in get_response_stream_async(context=combined_context, query=english_query, history=formatted_history): - if chunk_count == 0: - logger.info(f"First LLM chunk received in {time.perf_counter() - t6:.2f}s") - # If client disconnects, stop generating if await request.is_disconnected(): disconnected = True logger.info(f"Client disconnected during streaming after {chunk_count} chunks.") break + if chunk_count == 0: + logger.info(f"First LLM chunk received in {time.perf_counter() - t6:.2f}s") chunk_count += 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/api/routes/chat.py` around lines 157 - 164, The "First LLM chunk received" log is emitted before we confirm the client is still connected, so it can fire even when the client disconnected; move the first-chunk logging and any increment/usage of chunk_count to occur after the disconnected check (i.e., call await request.is_disconnected() first and only if False then set chunk_count == 0 handling and call logger.info). Update the loop around chunk handling in chat.py to reference chunk_count, request.is_disconnected(), and logger.info so the first-chunk message is logged only when a chunk is actually sent to a connected client.backend/app/services/database.py (1)
18-19: 타임아웃 하드코딩은 공통 설정으로 단일화하는 편이 안전합니다.Line 18의
30은 다른 경로의 타임아웃 상수와 쉽게 드리프트가 생길 수 있습니다. 설정값(예:settings)으로 단일화하면 운영 중 튜닝/변경이 훨씬 안전해집니다.♻️ 제안 예시
+POSTGREST_TIMEOUT = float(getattr(settings, "POSTGREST_TIMEOUT", 30.0)) ... -options = ClientOptions(postgrest_client_timeout=30) +options = ClientOptions(postgrest_client_timeout=POSTGREST_TIMEOUT)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/database.py` around lines 18 - 19, The hardcoded timeout 30 in the ClientOptions construction should be replaced with a centralized configuration value; change the call that constructs ClientOptions(postgrest_client_timeout=30) to use a settings constant (e.g., settings.POSTGREST_CLIENT_TIMEOUT or settings.DB_CLIENT_TIMEOUT) and ensure the settings value is used when calling create_client(supabase_url, supabase_key, options=options); update any import or settings reference required so ClientOptions and postgrest_client_timeout use the single source-of-truth timeout value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/api/routes/chat.py`:
- Around line 95-97: In the except asyncio.TimeoutError handler (the block that
currently calls logger.error and then yields {"event": "error", "data": "..."}),
replace logger.error(...) with logger.exception(...) so the timeout is logged
consistently with other handlers and includes the stack trace; leave the yield
{"event": "error", "data": "..."} behavior unchanged.
---
Nitpick comments:
In `@backend/app/api/routes/chat.py`:
- Around line 17-20: The semaphore concurrency hardcode (_db_rpc_semaphore =
asyncio.Semaphore(16)) should be replaced with a configurable value; add or read
a setting (e.g., DB_RPC_CONCURRENCY or a config entry) and use it to construct
_db_rpc_semaphore (keeping CHAT_TIMEOUT as-is), validate/parse the env/config as
int and provide a sensible default (16) if unset, and update any docstring or
comment near _db_rpc_semaphore to note it is configurable.
- Around line 157-164: The "First LLM chunk received" log is emitted before we
confirm the client is still connected, so it can fire even when the client
disconnected; move the first-chunk logging and any increment/usage of
chunk_count to occur after the disconnected check (i.e., call await
request.is_disconnected() first and only if False then set chunk_count == 0
handling and call logger.info). Update the loop around chunk handling in chat.py
to reference chunk_count, request.is_disconnected(), and logger.info so the
first-chunk message is logged only when a chunk is actually sent to a connected
client.
In `@backend/app/services/database.py`:
- Around line 18-19: The hardcoded timeout 30 in the ClientOptions construction
should be replaced with a centralized configuration value; change the call that
constructs ClientOptions(postgrest_client_timeout=30) to use a settings constant
(e.g., settings.POSTGREST_CLIENT_TIMEOUT or settings.DB_CLIENT_TIMEOUT) and
ensure the settings value is used when calling create_client(supabase_url,
supabase_key, options=options); update any import or settings reference required
so ClientOptions and postgrest_client_timeout use the single source-of-truth
timeout value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86e2a1d5-d89a-4fec-a513-6e0fa2b73251
📒 Files selected for processing (2)
backend/app/api/routes/chat.pybackend/app/services/database.py
Comment on lines
+95
to
+97
| except asyncio.TimeoutError: | ||
| logger.error(f"Database search timed out after {time.perf_counter() - t4:.2f}s") | ||
| yield {"event": "error", "data": "검색이 지연되고 있어요. 잠시 후 다시 시도해 주세요."} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: timeout except 블록의 로깅 API 확인
rg -n -C2 'except asyncio\.TimeoutError:|logger\.(error|exception)\(' backend/app/api/routes/chat.pyRepository: SanghunYun95/philo-rag
Length of output: 2296
데이터베이스 검색 타임아웃 핸들러의 로깅 일관성 개선 필요
Line 96의 logger.error를 logger.exception으로 변경해 주세요. 현재 코드는 같은 파일의 다른 TimeoutError 핸들러들(라인 57, 75, 215)과 달리 로깅 방식이 불일치합니다. 또한 logger.exception을 사용하면 스택 트레이스 정보를 포함하여 디버깅이 용이합니다.
제안 수정
except asyncio.TimeoutError:
- logger.error(f"Database search timed out after {time.perf_counter() - t4:.2f}s")
+ logger.exception(f"Database search timed out after {time.perf_counter() - t4:.2f}s")
yield {"event": "error", "data": "검색이 지연되고 있어요. 잠시 후 다시 시도해 주세요."}🧰 Tools
🪛 Ruff (0.15.2)
[warning] 96-96: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/routes/chat.py` around lines 95 - 97, In the except
asyncio.TimeoutError handler (the block that currently calls logger.error and
then yields {"event": "error", "data": "..."}), replace logger.error(...) with
logger.exception(...) so the timeout is logged consistently with other handlers
and includes the stack trace; leave the yield {"event": "error", "data": "..."}
behavior unchanged.
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
1 participant
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.
Summary by CodeRabbit
성능 개선 및 안정성 강화