Skip to content

Commit c9ea4ab

Browse files
lwgrayclaude
andcommitted
fix(cost): close only latest open run per project on end_experiment (#537)
The cost schema allows multiple ``runs`` rows per ``project_id`` (retries, repeated traversals — ``run_id`` is the PK, project views group/list runs). The bulk close in ``close_open_runs_for_project`` stamped every historical open row for the project with the just-finished experiment's ``ended_at`` and final counters, corrupting older runs. Replace with ``close_latest_open_run_for_project``: scope to the ``MAX(started_at)`` open run only. Older open rows are left to the periodic ``backfill_run_close_state.py`` script, which derives ``ended_at`` from each row's own ``MAX(token_events.timestamp)`` — the on-disk approximation that's correct per-run rather than a batch-overwrite from the live hook. Return type tightened from ``int`` (count) to ``Optional[str]`` (the closed run_id) so callers can log which row was touched. Caught by Codex P2 on PR #538. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent a4ac295 commit c9ea4ab

3 files changed

Lines changed: 92 additions & 47 deletions

File tree

src/cost_tracking/cost_store.py

Lines changed: 39 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -992,7 +992,7 @@ def close_run(
992992
self.conn.commit()
993993
return cur.rowcount > 0
994994

995-
def close_open_runs_for_project(
995+
def close_latest_open_run_for_project(
996996
self,
997997
project_id: str,
998998
*,
@@ -1001,46 +1001,59 @@ def close_open_runs_for_project(
10011001
completed_tasks: Optional[int] = None,
10021002
blocked_tasks: Optional[int] = None,
10031003
num_agents: Optional[int] = None,
1004-
) -> int:
1005-
"""Close every open run attributed to one project (Marcus #537).
1004+
) -> Optional[str]:
1005+
"""Close the most recently started open run for a project (#537).
1006+
1007+
Live-close hook for ``end_experiment``. The schema allows
1008+
multiple ``runs`` rows per ``project_id`` (retries, repeated
1009+
traversals), so a bulk close would stamp every historical
1010+
open row with the just-finished experiment's
1011+
``ended_at`` / counters — corrupting older runs.
10061012
1007-
Wraps :meth:`close_run` over every ``runs`` row where
1008-
``project_id`` matches and ``ended_at IS NULL``. Used as the
1009-
live-close hook from ``end_experiment``, where we know the
1010-
monitor's project_id but not the cost-tracking run_id.
1013+
Scope the close to the latest open run, identified by
1014+
``MAX(started_at)``. Older open rows for the same project are
1015+
left to the periodic ``backfill_run_close_state.py``, which
1016+
derives ``ended_at`` from each row's own
1017+
``MAX(token_events.timestamp)``.
10111018
10121019
The incoming ``project_id`` is normalized via
10131020
:func:`src.cost_tracking.cost_recorder.canonical_project_id`
10141021
so callers can pass either dashed UUID form (ProjectRegistry)
10151022
or dashless hex form (SQLiteKanban auto-discovery) — both
10161023
resolve to the dashless form actually stored in ``runs``.
1017-
Without this, the live close from ``end_experiment`` silently
1018-
no-ops whenever the monitor was handed a dashed project_id.
1024+
Without this, the live close silently no-ops whenever the
1025+
monitor was handed a dashed project_id.
10191026
10201027
Returns
10211028
-------
1022-
int
1023-
Number of runs closed.
1029+
Optional[str]
1030+
The closed ``run_id``, or ``None`` if the project had no
1031+
open runs.
10241032
"""
10251033
from src.cost_tracking.cost_recorder import canonical_project_id
10261034

10271035
normalized = canonical_project_id(project_id) or project_id
1028-
rows = self.conn.execute(
1029-
"SELECT run_id FROM runs WHERE project_id = ? AND ended_at IS NULL",
1036+
row = self.conn.execute(
1037+
"""
1038+
SELECT run_id FROM runs
1039+
WHERE project_id = ? AND ended_at IS NULL
1040+
ORDER BY started_at DESC
1041+
LIMIT 1
1042+
""",
10301043
(normalized,),
1031-
).fetchall()
1032-
closed = 0
1033-
for (run_id,) in rows:
1034-
if self.close_run(
1035-
run_id,
1036-
ended_at=ended_at,
1037-
total_tasks=total_tasks,
1038-
completed_tasks=completed_tasks,
1039-
blocked_tasks=blocked_tasks,
1040-
num_agents=num_agents,
1041-
):
1042-
closed += 1
1043-
return closed
1044+
).fetchone()
1045+
if row is None:
1046+
return None
1047+
(run_id,) = row
1048+
closed = self.close_run(
1049+
run_id,
1050+
ended_at=ended_at,
1051+
total_tasks=total_tasks,
1052+
completed_tasks=completed_tasks,
1053+
blocked_tasks=blocked_tasks,
1054+
num_agents=num_agents,
1055+
)
1056+
return run_id if closed else None
10441057

10451058
def record_price(self, price: ModelPrice) -> None:
10461059
"""Insert one ``model_prices`` row.

src/marcus_mcp/tools/experiments.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -252,18 +252,18 @@ async def end_experiment() -> Dict[str, Any]:
252252

253253
_cost_store = get_recorder().store
254254
final = result.get("final_metrics", {}) or {}
255-
closed = _cost_store.close_open_runs_for_project(
255+
closed_run_id = _cost_store.close_latest_open_run_for_project(
256256
project_id=project_id,
257257
ended_at=datetime.now(timezone.utc),
258258
total_tasks=final.get("total_tasks"),
259259
completed_tasks=final.get("total_task_completions"),
260260
blocked_tasks=final.get("total_blockers"),
261261
num_agents=final.get("total_registered_agents"),
262262
)
263-
if closed > 0:
263+
if closed_run_id:
264264
logger.info(
265-
"Closed %d open run(s) for project %s on end_experiment",
266-
closed,
265+
"Closed run %s for project %s on end_experiment",
266+
closed_run_id,
267267
project_id,
268268
)
269269
except Exception as _close_err: # pragma: no cover

tests/unit/cost_tracking/test_cost_store.py

Lines changed: 49 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1034,22 +1034,29 @@ def test_none_fields_preserve_existing_values(self, store: CostStore) -> None:
10341034
assert row[0] == 15 # original value preserved
10351035

10361036

1037-
class TestCloseOpenRunsForProject:
1038-
"""``close_open_runs_for_project`` is the bulk-close helper (#537)."""
1037+
class TestCloseLatestOpenRunForProject:
1038+
"""``close_latest_open_run_for_project`` is the live-close helper (#537)."""
10391039

1040-
def test_closes_all_open_runs_for_project(self, store: CostStore) -> None:
1041-
"""Two open runs for proj_x both get closed; closed runs are untouched."""
1040+
def test_closes_only_latest_open_run(self, store: CostStore) -> None:
1041+
"""Older open run is preserved; only MAX(started_at) is stamped.
1042+
1043+
Regression for Codex P2 on PR #538: the schema allows multiple
1044+
open runs per project (retries, repeated traversals). A bulk
1045+
close would stamp historical rows with the just-finished
1046+
experiment's ended_at + counters, corrupting them. The live
1047+
hook must scope to one row.
1048+
"""
10421049
store.record_run(
10431050
Run(
1044-
run_id="r1",
1051+
run_id="r_old",
10451052
project_id="proj_x",
10461053
project_name="x",
10471054
started_at=datetime(2026, 5, 13, 10, tzinfo=timezone.utc),
10481055
)
10491056
)
10501057
store.record_run(
10511058
Run(
1052-
run_id="r2",
1059+
run_id="r_new",
10531060
project_id="proj_x",
10541061
project_name="x",
10551062
started_at=datetime(2026, 5, 13, 11, tzinfo=timezone.utc),
@@ -1058,24 +1065,49 @@ def test_closes_all_open_runs_for_project(self, store: CostStore) -> None:
10581065
# Different project — must NOT be touched.
10591066
store.record_run(
10601067
Run(
1061-
run_id="r3",
1068+
run_id="r_other",
10621069
project_id="other",
10631070
project_name="o",
10641071
started_at=datetime(2026, 5, 13, 10, tzinfo=timezone.utc),
10651072
)
10661073
)
10671074

10681075
end = datetime(2026, 5, 13, 12, tzinfo=timezone.utc)
1069-
closed = store.close_open_runs_for_project("proj_x", ended_at=end)
1070-
assert closed == 2
1076+
closed_id = store.close_latest_open_run_for_project(
1077+
"proj_x", ended_at=end, total_tasks=8
1078+
)
1079+
assert closed_id == "r_new"
1080+
1081+
rows = {
1082+
run_id: (ended_at, total_tasks)
1083+
for run_id, ended_at, total_tasks in store.conn.execute(
1084+
"SELECT run_id, ended_at, total_tasks FROM runs ORDER BY run_id"
1085+
)
1086+
}
1087+
# Latest stamped with the experiment's counters.
1088+
assert rows["r_new"] == (end.isoformat(), 8)
1089+
# Older row left untouched for the periodic backfill.
1090+
assert rows["r_old"] == (None, None)
1091+
# Different project untouched.
1092+
assert rows["r_other"] == (None, None)
1093+
1094+
def test_returns_none_when_no_open_runs(self, store: CostStore) -> None:
1095+
"""Empty / fully-closed project returns None — caller treats as no-op."""
1096+
end = datetime(2026, 5, 13, 12, tzinfo=timezone.utc)
1097+
# Project never recorded.
1098+
assert store.close_latest_open_run_for_project("ghost", ended_at=end) is None
10711099

1072-
# proj_x's runs are closed; other's stays open.
1073-
rows = dict(
1074-
store.conn.execute("SELECT run_id, ended_at FROM runs ORDER BY run_id")
1100+
# Project with a row that's already closed.
1101+
store.record_run(
1102+
Run(
1103+
run_id="r_done",
1104+
project_id="proj_y",
1105+
project_name="y",
1106+
started_at=datetime(2026, 5, 13, 10, tzinfo=timezone.utc),
1107+
ended_at=datetime(2026, 5, 13, 11, tzinfo=timezone.utc),
1108+
)
10751109
)
1076-
assert rows["r1"] == end.isoformat()
1077-
assert rows["r2"] == end.isoformat()
1078-
assert rows["r3"] is None
1110+
assert store.close_latest_open_run_for_project("proj_y", ended_at=end) is None
10791111

10801112
def test_dashed_project_id_canonicalizes_to_dashless(
10811113
self, store: CostStore
@@ -1103,8 +1135,8 @@ def test_dashed_project_id_canonicalizes_to_dashless(
11031135

11041136
end = datetime(2026, 5, 13, 12, tzinfo=timezone.utc)
11051137
# Caller hands in the dashed form — must still close the row.
1106-
closed = store.close_open_runs_for_project(dashed, ended_at=end)
1107-
assert closed == 1
1138+
closed_id = store.close_latest_open_run_for_project(dashed, ended_at=end)
1139+
assert closed_id == "r_canon"
11081140

11091141
row = store.conn.execute(
11101142
"SELECT ended_at FROM runs WHERE run_id = ?", ("r_canon",)

0 commit comments

Comments
 (0)