fix(DATAGO-134113): Reduce API calls for Context Usage Indicator#1472
Open
amir-ghasemi wants to merge 5 commits into
Open
fix(DATAGO-134113): Reduce API calls for Context Usage Indicator#1472amir-ghasemi wants to merge 5 commits into
amir-ghasemi wants to merge 5 commits into
Conversation
Frontend was refetching context usage on every messages.length change (status bubbles, intermediate tool/peer messages flap several times per turn) and the post-response poller retried up to 4 times — combined, ~5–7 calls per chat turn. Drop the messageCount-driven effect; the TanStack Query key change handles the initial fetch and the poller already covers new-task arrival. Cap the poller at 2 attempts (1s, 3s). Backend now projects only the columns the endpoint reads from TaskModel (drops initial_request_text and ~10 other unused columns), adds DISTINCT ON (task_id) to the task-events lookup so fan-out within a task can never return more than one row, and projects (task_id, payload) only. ix_task_events_task_created already supports the new DISTINCT ON path optimally.
…adpool The body of get_session_context_usage is purely synchronous SQLAlchemy plus CPU work — no awaits. Declared as async def, it ran on the event loop and blocked it for the full duration of every DB round-trip (measured up to ~130 ms on the largest sessions in prod), stalling every other concurrent request on the worker. FastAPI auto-runs def handlers in its threadpool, so dropping the async keyword is sufficient — no asyncio.to_thread wrapping needed and no AsyncSession migration. Tests updated to call the now-sync handler directly (drop @pytest.mark.asyncio and the await on each call site). The same treatment likely applies to most other read-only handlers in this router; tracking that as separate follow-up since it crosses many endpoints.
|
❌ FOSSA Guard: Vulnerability (
|
.distinct(column) compiles to PostgreSQL-only DISTINCT ON and raises CompileError on SQLite/MySQL. The gateway DB officially supports all three (dependencies.init_database branches on dialect_name), and SQLite is the default for the WebUI gateway in configs/gateways/ webui.yaml — so any local-dev or self-hosted user with default config would have hit a 500 the first time their session had completed tasks. Unit tests didn't catch it because they use MagicMock and never compile real SQL; the prod check happened to run on PostgreSQL. Fix: keep the column projection (still saves payload weight on the TaskModel side) and the (task_id, created_time ASC) ORDER BY, but fold to the earliest request per task in Python — same as the pre-perf-commit code did. The dedup is essentially a no-op anyway (prod fan-out is ~1 request event per task), so no perf loss. Tests pin the projection (asserts the TaskModel query selects exactly the six columns the response builder reads, and that the heavy initial_request_text is not projected) and add a first-wins-per-task behavior test so the dedup ordering can't regress silently.
…cator Three tests pin the behavior changed by the perf patch: - poll fires at 1s and 3s after isResponding flips true→false - loop exits early once totalTasks >= baseline + 1 - bumping messageCount alone never triggers a refetch Two non-obvious setup details worth noting for future test authors: 1. vi.doMock + dynamic import live in a per-test loadComponent helper, not in beforeEach. With the number of doMocks here, putting them in beforeEach with the dynamic import in the same hook reliably trips the 10s hook timeout. Pattern matches RecentChatsList.test.tsx. 2. The lucide-react Proxy mock has to special-case ``then`` and well-known Symbols. Returning the Stub component for ``.then`` makes the dynamic import runtime mistake the resolved namespace for a thenable Promise, await it, and hang forever waiting for resolve to be called.
|
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
2 participants
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.
This pull request optimizes backend and frontend handling of session context usage, focusing on performance improvements and simplifying code. The most significant changes include making the backend context usage endpoint synchronous for better concurrency, optimizing SQL queries to reduce unnecessary data transfer, and refining frontend polling logic to minimize redundant API calls. Additionally, all related unit tests are updated to match the new synchronous API.
Backend performance and API improvements:
get_session_context_usagefromasync defto regulardef, so FastAPI runs it in a threadpool—this prevents blocking the event loop with synchronous database operations and improves concurrency. [1] [2]get_session_context_usageto select only the columns actually used, significantly reducing row payload and improving performance for long sessions.DISTINCT ONat the database layer and projecting only the necessary fields, further reducing data transfer and processing overhead.Frontend polling and API usage:
ContextUsageIndicatorcomponent to avoid refetching usage data on every message change; now, it only polls after a response is complete, with a capped number of attempts and increased delay to reduce unnecessary API calls and load. [1] [2]Test updates:
async defandpytest.mark.asynciodecorators to match the updated API. Also updated mocks to support new query chaining. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13]