Repository navigation
fix(raven-oncall): a campaign bills only its own jobs under a shared rounds directory - #553
Conversation
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: apply campaign ownership to OpenFOAM spend and prevent stale job adoption from concluded sibling directories.
Two concrete failures remain:
OpenFoamExecutor.spent_minutes()overrides the filtered implementation and still charges every job under the shared directory.- Reusing a concluded sibling's directory can collide on the deterministic idempotency key, causing the new campaign to inherit both an old bill and an old result.
Review coverage: I read AGENTS.md, CLAUDE.md, CONTEXT-MAP.md, and the applicable runtime context; inspected the full diff and all three commits; traced backend construction and spend callers, the watcher, ledger semantics, config-key generation, and submit/restart behavior; checked backward compatibility and the resource-admission architecture; and checked that existing tests were not weakened. git diff --check is clean. uv run pytest tests/test_agents_oncall_flow_*.py -q passed all 758 tests, but focused reproductions exposed the two gaps marked inline.
…rounds directory
`ProcessExecutor.spent_minutes` walked `{remote_dir}/jobs/*` and placed every
job it found on the campaign's timeline. Campaigns of one task usually declare
the same `remote_dir` -- an agent running a task in rounds points each round at
the same `runs/` -- so a later campaign was charged for every earlier one's runs
the moment it started. Measured 2026-09-11 on a shared 2xA800 box: five
campaigns shared one directory; the second-round campaign (budget 130) read
125.6 spent on its first look, 34.6 of it its own, and stopped with 142 real
minutes unspent and fifteen runs never submitted.
A job directory carries no campaign identity (its name's suffix is a digest of
the config), so ownership is read from the campaign's ledger. The executor
gains `restrict_spend_to(ledger)`; rows for jobs not in the ledger are skipped
before the timeline arithmetic, so the overlap rules for the campaign's own
concurrent runs are unchanged. Jobs this executor submitted itself are always
counted, so a run is billed before the ledger records it. `billing_only` in
`backends.py` hands the ledger over at the one seam every tool builds its
backend through; factories keep their one-argument shape, and a backend without
the seam (or a test double) is handed back untouched.
Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
…campaign keeps Even with spend read from the ledger, two campaigns writing rounds into one directory still mix their job trees. `ops_declare` now refuses a `remote_dir` that another campaign under the same ops home, not yet concluded, already declared. A concluded sibling is done writing there and does not count, and re-declaring the same campaign is not a collision. The refusal names the campaign holding the directory and says what to do, in the same voice as the rounds-inside-the-case refusal beside it. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
The resource-admission spec described the spend measure as a walk of the jobs directory; it now says the walk is filtered to the campaign's own ledger and why. The changelog carries the fix under Unreleased. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
250bbde to
d794cda
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the two existing review threads still reproduce and must be addressed.
This revision's only delta replaces the ledger attribute probe with runtime isinstance narrowing; I traced its callers and found no separate regression in that change. It does not alter either outstanding failure path, and both focused reproductions still fail with the same measured results.
Coverage for this revision: I rechecked the repository rules and applicable context, the full branch diff and new delta/history, backend and tool callers, ledger/config-key/restart compatibility, the resource-admission architecture, and test coverage; no existing tests were weakened. git diff --check github/main...HEAD is clean. uv run pytest tests/test_agents_oncall_flow_*.py -q passed all 758 tests. The original two threads remain open because neither is settled.
…tovers Two gaps in the ownership filter, both found in review. OpenFoamExecutor inherits restrict_spend_to, so billing_only appears to configure it, but it overrides spent_minutes and its own loop walked every scanned row. An owned 20-core-minute case beside a sibling's 80 read 100. It now asks the same question the base loop does, before it records the width: _cores and the timeline are both read back by the caller, and a sibling in either one is the same cross-campaign bill somewhere else. A concluded sibling's directory was treated as free, but concluding does not remove its jobs. A job directory is named from a digest of its config and carries no campaign, so two campaigns running the same trial name the same directory: the newcomer's ledger records that key as its own, the filter reads the predecessor's minutes as this campaign's, and submit finds the old result.json and calls the trial done instead of running it. A concluded sibling now keeps its directory as long as its ledger holds a record; one that never submitted anything left nothing to adopt and still steps aside. A ledger that cannot be parsed counts as occupied, which is the safe direction. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
7050e45 to
5ed0d5d
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The new commit fixes both prior findings and adds direct regressions for them. I replied to and resolved both threads after verifying the fixes.
Coverage: I rechecked the repository rules and applicable context, the full branch diff and revision history, all affected backend/tool callers, ledger and deterministic-key behavior, restart/backward-compatibility paths, the resource-admission architecture, and the tests for weakening or missing assertions. I found no new defect. git diff --check github/main...HEAD is clean, and uv run pytest tests/test_agents_oncall_flow_*.py -q passed all 761 tests.
3 participants
A campaign asked the shared rounds directory how much had been spent, and the
directory answered for every job in it, including jobs its sibling campaigns
had submitted. Rounds that keep one directory therefore bill each other, and
the bill is cumulative: round three opens already charged for rounds one and
two.
Measured on a run of 2026-09-11. Five campaigns declared the same rounds
directory. Round two had a budget of 130 and had actually used 34.6, but the
eighteen jobs the earlier rounds had left in the directory brought the reading
to 125.6, so the remaining 4.4 would not cover one more trial at 5.9 and the
round stopped. 142 GPU-minutes went unspent and that round's multi-seed
conclusion was lost. A sibling run that gave each round its own directory was
not affected, which is why this reads as a directory-hygiene problem until you
look at the meter.
Two changes. Spend is now counted from the campaign's own ledger, so a job a
sibling submitted is not billed here; the filter sits above the existing
timeline arithmetic, so same-device overlap is still charged once and two
cards in parallel are still charged twice. And declare refuses a rounds
directory a live sibling campaign already keeps, naming the sibling, so the
two rounds do not land in one directory in the first place.
Type: fix
Verification:
(test_the_products_tool_face_equals_the_forks_config_intent, a missing
optional a2a_client dependency on this machine) also fails on 0464aa1, so
this adds six passing tests and no new failures.
trials overlapping on one device are still billed 20 minutes once; a job
this executor just submitted is billed before the ledger has caught up (no
refund window); billing_only leaves a foreign backend untouched; declare
refuses a directory a live sibling keeps; a concluded sibling, and the
campaign itself, do not count as a clash.
reads 125.6 unfiltered, which is 34.6 of its own plus 91.0 of its siblings.
Risk: low for the meter, moderate for declare. The meter can only ever bill
less than before, and a run that already gives each round its own directory
reads the same. Declare now refuses something it used to accept, so a caller
that deliberately shares a directory between live campaigns will see an error
naming the sibling; concluded siblings and re-declaring the same campaign are
both still fine.