Skip to content

Commit bf1223b

Browse files
committed
perf(tasks): maintain task browse summaries on trial writes
One summary row per task version (exact card counters, status buckets, model-grouped cost inputs, last_run_at), refreshed inside the same transaction as every trial-population mutation: create/import, start/reset, completion, cancellation, retry/supersede, scoped deletion, experiment deletion, and default-version selection. Refreshes serialize per version with sorted transaction-scoped advisory locks because concurrent trial inserts hold KEY SHARE FK locks on task_versions. No reader change and no historical scan: the browse endpoint still aggregates on demand, and rows exist only for versions written to after deploy. The read cutover and historical population land separately. Split 1/4 of the #1152 re-land.
1 parent eb98b01 commit bf1223b

17 files changed

Lines changed: 581 additions & 29 deletions

AGENTS.md

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -351,6 +351,26 @@ usage across every trial owned by the experiment, including older versions,
351351
superseded retries, probes, and soft-deleted trials. Its `billed_*` cost and
352352
token fields are the billed-user subset used by the frontend's New spend tile.
353353

354+
### Task Browser Summary
355+
356+
`task_version_browse_summaries` keeps one bounded aggregate row per task
357+
version: exact card counters, status buckets, model-grouped cost inputs, and
358+
`last_run_at`. It exists so the default `GET /tasks/browse` page can select
359+
and order tasks without scanning organization trial history; until the read
360+
path cuts over to it, the rows are maintained but unread.
361+
362+
Summary scope matches normal task cards: exclude probes, superseded attempts,
363+
soft-deleted trials, and `combine:` copies. Any mutation that changes that
364+
population or its metrics must call
365+
`refresh_task_browse_summaries` inside the same transaction. This includes
366+
trial create/import, start/reset, completion, cancellation, retry/supersede,
367+
scoped deletion, and default-version selection.
368+
369+
Refreshes serialize per version with sorted transaction-scoped PostgreSQL
370+
advisory locks; do not replace those locks with `FOR UPDATE` on
371+
`task_versions`, because concurrent trial inserts already hold foreign-key
372+
`KEY SHARE` locks and lock upgrades can deadlock.
373+
354374
---
355375

356376
## `oddish/` — Core Package

backend/tests/test_browse_search.py

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
- the free-text grammar: terms AND in any order, "quoted phrase", -exclusion
88
"""
99

10+
import asyncio
1011
import os
1112

1213
import pytest
@@ -18,6 +19,8 @@
1819

1920
import models # noqa: F401 registers cloud tables on the shared Base
2021
from oddish.core.endpoints import browse_tasks_core
22+
from oddish.core.endpoints.deletion import delete_experiment_core
23+
from oddish.core.task_browse_summary import refresh_task_browse_summaries
2124
from oddish.db.models import Base
2225

2326
URL = os.environ.get("ODDISH_DATABASE_URL")
@@ -139,6 +142,117 @@ async def test_combine_copies_excluded_from_browse():
139142
await engine.dispose()
140143

141144

145+
async def test_experiment_delete_refreshes_surviving_task_summaries():
146+
engine = create_async_engine(URL)
147+
maker = async_sessionmaker(engine, expire_on_commit=False)
148+
try:
149+
await _setup_combine(engine)
150+
async with maker() as session:
151+
# A real (non-combine) exp-b trial so t-c outlives exp-a's delete.
152+
await session.execute(
153+
text(
154+
"""
155+
INSERT INTO trials (
156+
id, name, task_id, task_version_id, experiment_id, org_id,
157+
agent, model, provider, queue_key, timeout_minutes,
158+
environment, harbor_config, status, origin, is_probe,
159+
reward, finished_at, attempts, max_attempts,
160+
heartbeat_failure_count, has_trajectory, created_at, updated_at
161+
) VALUES (
162+
'tr-b', 'tr-b', 't-c', 'v-c', 'exp-b', 'org1',
163+
'claude', 'sonnet', 'anthropic', 'q', 30, 'modal',
164+
'{}'::jsonb, 'SUCCESS', 'oddish', false, 1.0, NOW(),
165+
1, 6, 0, false, NOW(), NOW()
166+
)
167+
"""
168+
)
169+
)
170+
await refresh_task_browse_summaries(session, ["v-c"])
171+
await session.commit()
172+
summary_row = text(
173+
"""
174+
SELECT total_trials, completed_trials
175+
FROM task_version_browse_summaries
176+
WHERE task_version_id = 'v-c'
177+
"""
178+
)
179+
before = (await session.execute(summary_row)).one()
180+
assert before == (2, 2) # tr-src + tr-b; the combine copy excluded
181+
182+
await delete_experiment_core(
183+
session, experiment_id="exp-a", org_id=ORG
184+
)
185+
await session.commit()
186+
after = (await session.execute(summary_row)).one()
187+
188+
# The surviving task's summary must drop exp-a's tombstoned trial in
189+
# the delete transaction itself, and exp-b's combine copy must stay
190+
# excluded.
191+
assert after == (1, 1)
192+
finally:
193+
await engine.dispose()
194+
195+
196+
async def test_summary_refresh_serializes_concurrent_settlements():
197+
engine = create_async_engine(URL)
198+
maker = async_sessionmaker(engine, expire_on_commit=False)
199+
try:
200+
await _setup_combine(engine)
201+
async with maker() as first, maker() as second:
202+
insert_trial = text(
203+
"""
204+
INSERT INTO trials (
205+
id, name, task_id, task_version_id, experiment_id, org_id,
206+
agent, model, provider, queue_key, timeout_minutes,
207+
environment, harbor_config, status, origin, is_probe,
208+
reward, finished_at, attempts, max_attempts,
209+
heartbeat_failure_count, has_trajectory, created_at, updated_at
210+
) VALUES (
211+
:trial_id, :trial_id, 't-c', 'v-c', 'exp-a', 'org1',
212+
'claude', 'sonnet', 'anthropic', 'q', 30, 'modal',
213+
'{}'::jsonb, 'SUCCESS', 'oddish', false, 1, NOW(),
214+
1, 6, 0, false, NOW(), NOW()
215+
)
216+
"""
217+
)
218+
# Both inserts hold a KEY SHARE FK lock on v-c. Summary
219+
# serialization must not try to upgrade either lock to FOR UPDATE.
220+
await first.execute(insert_trial, {"trial_id": "tr-concurrent-a"})
221+
await second.execute(insert_trial, {"trial_id": "tr-concurrent-b"})
222+
# Generous budget: this only has to prove the holder does not
223+
# BLOCK (a lock-ordering bug hangs it forever); a tight budget
224+
# would flake on slow CI runners.
225+
await asyncio.wait_for(
226+
refresh_task_browse_summaries(first, ["v-c"]), timeout=5.0
227+
)
228+
229+
second_refresh = asyncio.create_task(
230+
refresh_task_browse_summaries(second, ["v-c"])
231+
)
232+
with pytest.raises(asyncio.TimeoutError):
233+
await asyncio.wait_for(asyncio.shield(second_refresh), timeout=0.1)
234+
235+
await first.commit()
236+
await second_refresh
237+
await second.commit()
238+
239+
async with maker() as check:
240+
row = (
241+
await check.execute(
242+
text(
243+
"""
244+
SELECT total_trials, completed_trials
245+
FROM task_version_browse_summaries
246+
WHERE task_version_id = 'v-c'
247+
"""
248+
)
249+
)
250+
).one()
251+
assert row == (3, 3)
252+
finally:
253+
await engine.dispose()
254+
255+
142256
async def test_search_wildcards_are_literals():
143257
engine = create_async_engine(URL)
144258
maker = async_sessionmaker(engine, expire_on_commit=False)

oddish/src/oddish/core/endpoints/deletion.py

Lines changed: 29 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
get_trial_for_org_core,
1414
_reset_task_verdict,
1515
)
16+
from oddish.core.task_browse_summary import refresh_task_browse_summaries
1617
from oddish.db import (
1718
AnalysisStatus,
1819
ExperimentModel,
@@ -168,13 +169,18 @@ async def delete_task_core(
168169
# Scoped delete: only this experiment's trials + the join row.
169170
scoped_trial_rows = (
170171
await session.execute(
171-
select(TrialModel.id, TrialModel.trial_s3_key).where(
172+
select(
173+
TrialModel.id,
174+
TrialModel.trial_s3_key,
175+
TrialModel.task_version_id,
176+
).where(
172177
TrialModel.task_id == resolved_task_id,
173178
TrialModel.experiment_id == experiment_id,
174179
)
175180
)
176181
).all()
177182
scoped_trial_ids = [row[0] for row in scoped_trial_rows]
183+
scoped_version_ids = {row[2] for row in scoped_trial_rows if row[2]}
178184

179185
# Check that this task really belongs to the given experiment.
180186
link_exists = await session.scalar(
@@ -264,6 +270,7 @@ async def delete_task_core(
264270
if task is not None:
265271
_reset_task_verdict(task)
266272

273+
await refresh_task_browse_summaries(session, scoped_version_ids)
267274
return {
268275
"s3_prefixes": [],
269276
"deleted": {
@@ -349,17 +356,16 @@ async def unlink_task_from_experiment_core(
349356

350357
# Tombstone this experiment's trials for the task (cancel their live
351358
# worker_jobs first so workers stop heart-beating and release slots).
352-
scoped_trial_ids = [
353-
row[0]
354-
for row in (
355-
await session.execute(
356-
select(TrialModel.id).where(
357-
TrialModel.task_id == resolved_task_id,
358-
TrialModel.experiment_id == experiment_id,
359-
)
359+
scoped_trial_rows = (
360+
await session.execute(
361+
select(TrialModel.id, TrialModel.task_version_id).where(
362+
TrialModel.task_id == resolved_task_id,
363+
TrialModel.experiment_id == experiment_id,
360364
)
361-
).all()
362-
]
365+
)
366+
).all()
367+
scoped_trial_ids = [row[0] for row in scoped_trial_rows]
368+
scoped_version_ids = {row[1] for row in scoped_trial_rows if row[1]}
363369
harvest = _CancelHarvest()
364370
if scoped_trial_ids:
365371
await _cancel_worker_jobs_for_trials(
@@ -435,6 +441,7 @@ async def unlink_task_from_experiment_core(
435441
org_id=task_org_id,
436442
)
437443

444+
await refresh_task_browse_summaries(session, scoped_version_ids)
438445
return {
439446
"s3_prefixes": [],
440447
"deleted": {
@@ -620,12 +627,13 @@ async def delete_experiment_core(
620627
or_(TrialModel.org_id == org_id, TrialModel.org_id.is_(None))
621628
)
622629

623-
scoped_trial_ids = [
624-
row[0]
625-
for row in (
626-
await session.execute(select(TrialModel.id).where(*trial_where))
627-
).all()
628-
]
630+
scoped_trial_rows = (
631+
await session.execute(
632+
select(TrialModel.id, TrialModel.task_version_id).where(*trial_where)
633+
)
634+
).all()
635+
scoped_trial_ids = [row[0] for row in scoped_trial_rows]
636+
scoped_version_ids = {row[1] for row in scoped_trial_rows if row[1]}
629637

630638
# Task-level QA/VERDICT jobs are cancelled in the survival loop below,
631639
# ONLY for tasks that actually die with this experiment -- a task alive
@@ -720,6 +728,9 @@ async def delete_experiment_core(
720728
_reset_task_verdict(task)
721729
_clear_stale_task_pipeline_status(task)
722730

731+
# Tasks that survive via other experiments keep their cards; their
732+
# summaries must drop the trials this delete just tombstoned.
733+
await refresh_task_browse_summaries(session, scoped_version_ids)
723734
return {
724735
"s3_prefixes": [],
725736
"deleted": {
@@ -1055,6 +1066,7 @@ async def delete_trial_core(
10551066
.values(deleted_at=utcnow())
10561067
.execution_options(synchronize_session=False)
10571068
)
1069+
await refresh_task_browse_summaries(session, [trial.task_version_id])
10581070

10591071
# Task aggregates (total/completed/failed) are derived from the
10601072
# remaining trials -- the soft-delete filter excludes this one

oddish/src/oddish/core/endpoints/task_detail.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
fetch_trial_queue_info,
1515
fetch_visible_worker_jobs,
1616
)
17+
from oddish.core.task_browse_summary import refresh_task_browse_summaries
1718
from oddish.core.tags.projection import (
1819
list_direct_version_tags,
1920
list_effective_user_tags_for_task_versions,
@@ -110,6 +111,7 @@ async def set_task_default_version_core(
110111
# raw SQL in the same transaction.
111112
await session.flush()
112113
await recompute_task_browse_projection(session, task_id=task.id)
114+
await refresh_task_browse_summaries(session, [version_row.id])
113115
return TaskVersionResponse.model_validate(version_row)
114116

115117

oddish/src/oddish/core/endpoints/trials.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
read_trial_trajectory,
2323
)
2424
from oddish.core.verdict_state import abandon_verdict
25+
from oddish.core.task_browse_summary import refresh_task_browse_summaries
2526
from oddish.db import (
2627
AnalysisStatus,
2728
TaskModel,
@@ -669,6 +670,7 @@ async def retry_trial_core(
669670
WorkerJobStatus.CANCELLED: "skipped",
670671
}.get(final_status, "queued")
671672

673+
await refresh_task_browse_summaries(session, [new_trial.task_version_id])
672674
await session.commit()
673675
return {
674676
"status": status_label,

oddish/src/oddish/core/ingest/trial_imports.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444

4545
from oddish.config import normalize_model_id, settings
4646
from oddish.core.harbor_artifacts import build_trial_result
47+
from oddish.core.task_browse_summary import refresh_task_browse_summaries
4748
from oddish.core.trial_facets import facet_rows_for_trial, record_trial_facets
4849
from oddish.db import (
4950
ExperimentModel,
@@ -372,6 +373,7 @@ async def initialize_trial_import(
372373
from oddish.queue import maybe_start_qa_stage
373374

374375
await maybe_start_qa_stage(session, trial_id)
376+
await refresh_task_browse_summaries(session, [task_version_id])
375377

376378
await session.commit()
377379

oddish/src/oddish/core/quota_enforcement.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -290,6 +290,11 @@ async def cancel_trials_if_quota_reached(
290290
"worker_targets": worker_targets,
291291
}
292292
)
293+
from oddish.core.task_browse_summary import refresh_task_browse_summaries
294+
295+
await refresh_task_browse_summaries(
296+
session, (trial.task_version_id for trial in trials)
297+
)
293298
await _reconcile_cancelled_tasks(session, result)
294299
return result
295300

0 commit comments

Comments
 (0)