Repository navigation
feat(llc): a stalled run is its own status, and the sweep locks what it claims (#16817) - #17696
Merged
Merged
Conversation
…it claims (#16817) Three of #16817's ACs. The release of what a run held is #16818's and is not touched here. AC4 -- a stalled run is distinguishable, not inferred. The sweep wrote `LLCRunStatus.TIMEOUT`, which four sites set: http_adapter, subprocess_base, heartbeat_scheduler and the sweep itself. So "which runs stalled?" meant matching a formatted, human-readable `error` string -- inference, and it breaks on a re-wording. `STALLED` is a new member; `error` still carries the detail. This reverses a decision recorded in the sweep's own docstring, so the reason is recorded too. The original reused TIMEOUT because that "keeps the state machine as it is". That cost is close to zero and was probably overestimated: `status` is `sa.String(32)` rather than a native enum, so no migration, and `is_terminal` is a deny-list over {QUEUED, RUNNING}, so a new terminal member needs no edit there. The thing traded away was what AC4 asks for. AC5 -- `FOR UPDATE SKIP LOCKED`. Two beat workers overlapping today would write the same terminal status, so the race is invisible. #16818 adds the release of claims, assignments and workspace leases to this path, and the same race then releases each holding twice. The lock belongs here BEFORE the side effects that need it: a defect armed by its own dependency is cheaper to prevent than to diagnose. The scan -- the sweep loaded EVERY non-terminal run and compared ages in Python. The existing comment's NULL reasoning was right and the implementation did not follow it; `COALESCE(started_at, created_at) <= cutoff` is that same reasoning expressed where an index can serve it. Not a reversal of a documented decision -- the decision was already this, in prose. The query moved into `_stalled_candidates()` so both properties are assertable without a database. Mutation-checked, each failing exactly one test and the right one: write TIMEOUT again -> the sweep's behavioural test fails drop SKIP LOCKED -> the lock test fails drop locking entirely -> the lock test fails age filter back in Python -> the in-SQL test fails Two notes on the tests themselves. The lock assertion compiles against `postgresql.dialect()`, because the default dialect renders FOR UPDATE and silently drops SKIP LOCKED -- asserting on the generic rendering would have reported a lock clause production never gets. And `_async_sweep` is now driven through a fake session: every other AC4 test passes while the sweep writes whatever it likes, because the enum member existing is not the sweep using it. `test_the_error_says_the_sweep_decided_it` kept its name and lost its premise -- it used to state that the error was "the only place that distinguishes them", which was true and was the defect. Refs #16817, #16818
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThe sweep now selects overdue queued or running runs in SQL, skips rows locked by another worker, and marks selected runs as ChangesStalled Run Handling
Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to Agents can currently mark their own runs as stalled without a sweep decision. Restrict that status before merging, and address the sweep query’s indexing risk as run counts grow. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to An agent can now report “stalled” for its own run, even though that outcome is intended to mean the sweep made the decision. Run and tenant scoping limit the exposure, but the distinction could mislead consumers of the new status. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: eaa5bab7-c051-4acb-b111-6aa794cc836a
📒 Files selected for processing (3)
autobot-backend/llc/models/enums.pyautobot-backend/llc/scheduler/stalled_run_sweep.pyautobot-backend/llc/scheduler/stalled_run_sweep_test.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
| # TIMEOUT alone cannot say which happened and a consumer had to match the | ||
| # `error` text to find out. Terminal by construction: `is_terminal` is a | ||
| # deny-list over {QUEUED, RUNNING}, so a new member needs no edit there. | ||
| STALLED = "stalled" |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '150,175p' autobot-backend/llc/models/enums.py
sed -n '320,400p' autobot-backend/llc/api/agent_api.py
rg -n 'STALLED|report_heartbeat|LLCRunStatus\(' autobot-backend/llcRepository: mrveiss/AutoBot-AI
Length of output: 7887
🏁 Script executed:
set -eu
rg -n -A35 -B12 'class HeartbeatReport|HeartbeatReport\s*=' autobot-backend/llc
sed -n '110,165p' autobot-backend/llc/scheduler/stalled_run_sweep.py
sed -n '165,215p' autobot-backend/llc/scheduler/stalled_run_sweep.py
sed -n '340,445p' autobot-backend/llc/tests/test_agent_stub_routes_15905.py
sed -n '130,160p' autobot-backend/llc/models/enums.pyRepository: mrveiss/AutoBot-AI
Length of output: 12368
Reject STALLED in agent heartbeat reports.
HeartbeatReport.status is an unrestricted string, and report_heartbeat accepts every value in LLCRunStatus. When an agent submits status="stalled", the route writes that terminal status and finished_at to the agent’s run. The run then cannot satisfy the sweep’s non-terminal and unfinished-run conditions. Reject STALLED in this route, but keep it available to stalled_run_sweep.
Suggested fix
try:
status = LLCRunStatus(body.status)
except ValueError as exc:
raise HTTPException(status_code=422, detail=f"Unknown run status {body.status!r}") from exc
+ if status is LLCRunStatus.STALLED:
+ raise HTTPException(status_code=422, detail="Agents cannot report stalled runs")
+
try:
run_uuid = uuid.UUID(body.run_id)
Comment on lines
+126
to
+128
| sql = _compiled(sweep).lower() | ||
| assert "coalesce" in sql | ||
| assert "started_at" in sql and "created_at" in sql |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '90,150p' autobot-backend/llc/scheduler/stalled_run_sweep.py
sed -n '95,210p' autobot-backend/llc/scheduler/stalled_run_sweep_test.pyRepository: mrveiss/AutoBot-AI
Length of output: 7230
Assert the cutoff predicate, not only its identifiers.
The current assertions can pass if the COALESCE age predicate is removed. The timestamp names also remain in the selected columns, and the sweep-outcome tests do not inspect the submitted statement.
Assert that the compiled WHERE clause compares COALESCE(started_at, created_at) with the cutoff.
Suggested test assertion
sql = _compiled(sweep).lower()
- assert "coalesce" in sql
- assert "started_at" in sql and "created_at" in sql
+ assert re.search(
+ r"coalesce\([^)]*started_at[^)]*,[^)]*created_at[^)]*\)\s*<=\s*",
+ sql,
+ )🧰 Tools
🪛 Ruff (0.16.6)
[warning] 128-128: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
| # #16817: that reasoning was right and the implementation did not follow it -- | ||
| # the comparison ran in Python over EVERY non-terminal row, so each sweep scanned | ||
| # the whole live population to act on a few. COALESCE puts the same rule in SQL. | ||
| age_anchor = func.coalesce(LLCHeartbeatRun.started_at, LLCHeartbeatRun.created_at) |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Inspect repository-owned index definitions without running repository code.
rg -n -i -C 3 'llc_heartbeat_runs|coalesce\s*\(' autobot-backend/llc -g '*.py' -g '*.sql'Repository: mrveiss/AutoBot-AI
Length of output: 14547
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- heartbeat model ---'
sed -n '1,220p' autobot-backend/llc/models/heartbeat_run.py
printf '%s\n' '--- candidate migration/index files ---'
git ls-files | rg '(^|/)(alembic|migrations|migration|.*heartbeat.*|.*schema.*)' | head -200
printf '%s\n' '--- repository index definitions and heartbeat columns ---'
rg -n -i -C 4 'llc_heartbeat_runs|started_at|created_at|index\s*\(|Index\s*\(|coalesce\s*\(' --glob '*.py' --glob '*.sql' --glob '*.yaml' --glob '*.yml' --glob '*.json' . | head -500Repository: mrveiss/AutoBot-AI
Length of output: 40758
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- initial heartbeat migration ---'
sed -n '1,260p' autobot-backend/migrations/versions/20260523_033_llc_heartbeat_runs.py
printf '%s\n' '--- all migration references to the heartbeat table or timestamp indexes ---'
rg -n -i -C 3 'llc_heartbeat_runs|ix_.*(started|created)|create_index|coalesce' autobot-backend/migrations autobot-backend/database --glob '*.py' --glob '*.sql'Repository: mrveiss/AutoBot-AI
Length of output: 41429
Add an index for the age predicate.
LLCHeartbeatRun filters on COALESCE(started_at, created_at). The existing column indexes do not index this expression. PostgreSQL can therefore evaluate the age condition across the rows that pass the status filter instead of using a timestamp range scan.
Add a matching expression index in a migration, or rewrite the predicate as separate started_at and created_at branches so PostgreSQL can use the column indexes.
mrveiss
added a commit
that referenced
this pull request
Sep 28, 2026
…v0.10.0 (#17717) (#17718) * ci(release): baseline five squash-stamped identity trailers blocking v0.10.0 (#17717) #17717 promotes main -> release and fails "No commit trailers" on commits already landed. Per-PR scans passed -- including #17696's own -- because the gate compares BASE..HEAD and the trailer does not exist until GitHub's squash-merge creates it. Only a promotion re-scans the landed range, so the check runs on an object no PR ever showed it. DERIVED AND VERIFIED, NOT TAKEN FROM A LIST. The run log reports ten "Offending commit:" lines and nine unique SHAs. The tenth is the workflow's own printf template -- `Offending commit: %s\n' "${bad[@]}"` -- echoed into the log. Nine real commits, and a count taken from the line count would have been the measurement's artefact rather than the population. The nine are TWO classes, established by reading each commit's trailers rather than assuming one cause: five Co-authored-by: t <t@example.invalid> four Co-authored-by: Claude <noreply@anthropic.com> ONLY THE FIVE ARE BASELINED. They are the same cause as the existing `test <test@test>` entries, a second time: a local user.name/user.email override shadowed the correct global identity, branch commits carried it, and the squash stamped it onto the result. `t <t@example.invalid>` is not a person and claims no attribution. The source is CLOSED -- no worktree config and no global config resolves user.name to `t`, checked rather than assumed. That check is what makes the class bounded at five, and the header says so, because the previous instance's header claimed the override was "since corrected" and a different one appeared anyway. The claim was true and insufficient; the check is the thing. THE FOUR ARE HELD, AND THE HEADER SAYS WHY. `Co-authored-by: Claude` is a real attribution claim -- exactly what no-commit-trailers.yml (#13654) exists to reject -- not a misconfigured identity. The reasoning that justifies the five does not reach it, so folding it in would exempt the thing the rule is for on a ruling about something else. A reader of this file must not conclude that a `Claude` co-author is baselinable. Its source is also not closed, which is why it is a separate question rather than a longer list: the agent harness instructs sessions to append that trailer while this project's rules forbid it and this gate enforces them. Until that is resolved at the source, new instances keep landing and a baseline is treating the symptom. Filed separately. THE TOP RULE IS NARROWED, NOT SUSPENDED. It said entries may only be removed. It now names the one class an addition is permitted for -- a trailer no branch commit carried, created by the squash, found only at promotion, on a history that cannot be rewritten -- and says explicitly that a trailer which WAS on a branch commit does not qualify, because that one was fixable and the fix is to fix it. Verified the gate's own parse: of the nine, exactly the five appear in `sed 's/#.*//' | tr -d '[:blank:]'`, and the four named in the header comment do not. The workflow is untouched. Refs #17717 Refs #13654 * ci(release): baseline the four Claude-trailer commits, and correct the class-1 cause (#17717) Two corrections to the first commit on this branch, both because the original was wrong about a fact it asserted. ALL NINE ARE NOW BASELINED, by explicit owner ruling. The first commit carried five and held four; the ruling reached both this session and the coordinator's and now agrees. The class-2 paragraph is unchanged in substance -- it still says a `Co-authored-by: Claude` trailer is the thing the gate exists to reject, is exempted only because `main` cannot be rewritten, and is NOT a precedent -- and it now also says its source is open, which makes it worse than class 1 rather than merely different. CLASS 1 WAS NOT A MISCONFIGURED USER IDENTITY. The first commit said the source was closed because no worktree config and no global config resolves `user.name` to `t`. That measurement was correct and about the wrong thing. `t <t@example.invalid>` is this repository's TEST-FIXTURE identity -- ten files carry it, eight run `git commit`, two of those under the pre-push hook. The reason it reaches the real repository is that an inherited `GIT_DIR` OUTRANKS `-C` and `cwd=`; `hooks_path_override_15961_test.py:450` records learning that the hard way. So no config override was ever there to find, and checking config is how you fail to find it. Three of the fixture files write the identity with `git config` rather than a per-invocation `-c`. Worktrees share one config through the common dir, so a single unscrubbed write sets it for every worktree at once -- which is the shape of five commits across five branches in one afternoon. WHAT I VERIFIED RATHER THAN RELAYED: every one of those sites currently scrubs. All eight committing tests pass `scrubbed_git_env()`, and all three config writers unset the ambient variables -- checked per file, not inferred from the helper existing. So no current test leaks, and the header says that instead of claiming the source is closed. The open window is a NEW fixture that commits or writes config, run before its scrub is added. That is what happened here: 969c195 is the commit that introduced one such test. Source is #15353, open; detection gap #16124; release-facing home #16268, which already carries 18 from an earlier promotion. So this baseline is the third containment of one root cause -- 22, then 18, now 9 -- and the header says so, because whoever picks up #15353 should be able to find the cost of not doing it in one place. Gate parse verified against the file as committed: 31 entries, all nine present. Refs #17717 Refs #13654 Refs #15353
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.
Thinking Path
Three of #16817's seven ACs. AC2, and the release halves of AC6 and AC7, are #16818's by
design — the sweep's own docstring says so, and #16818 is now v0.9.0 alongside this. Hence
Refs, notCloses: partial delivery closes nothing.The issue's problem statement is stale and I have corrected it on the issue. It says
"
llc/scheduler/contains one sweep,project_disposal_sweep.py". There are now eleven,including
stalled_run_sweep.pyandliveness_monitor.py— the sweep itself landed in8d2972a296+a01653a8ea. So AC1, AC3 and AC7's log half were already delivered before this PR;what remained was AC4, AC5, and an efficiency defect underneath them.
AC4 turned out to reverse a decision recorded in the sweep's own docstring, so I checked
before changing it rather than after. The original reused
TIMEOUTbecause that "keeps the statemachine as it is", putting the distinction in
error. That cost is close to zero and wasprobably overestimated:
statusissa.String(32)rather than a native enum, so no migration;and
is_terminal()is a deny-list over{QUEUED, RUNNING}, so a new terminal member needs noedit there. What the decision traded away is what AC4 asks for.
AC5 is the one to read first, because of its timing. A missing
SKIP LOCKEDis harmless whilethe write is an idempotent status set — and becomes a double release the moment #16818 adds
claims, assignments and workspace leases to this path. The defect is armed by the very issue this
one is waiting on. Fixing it now costs a clause.
What Changed
AC4 —
LLCRunStatus.STALLED. The sweep wroteTIMEOUT, which four sites set:http_adapter.py:84,subprocess_base.py:406,heartbeat_scheduler.py:1062, and the sweep. So"which runs stalled?" meant matching a formatted, human-readable
errorstring — inference, andit breaks on a re-wording.
errorstill carries the detail;statusnow carries the fact.AC5 —
.with_for_update(skip_locked=True). Two beat workers partition rather than collide, sothe lock is in place before the side effects that will need it.
The scan. The sweep selected every non-terminal run and compared ages in Python. The
existing comment's NULL reasoning was right and the implementation did not follow it — this is
COALESCE(started_at, created_at) <= cutoff, the same rule expressed where an index can serve it.Not a reversal of a documented decision: the decision was already this, in prose.
The statement moved into
_stalled_candidates()so both properties are assertable without adatabase.
test_the_error_says_the_sweep_decided_itkept its name and lost its premise. It used tostate that the error text was "the only place that distinguishes them" — which was true, and was
the defect.
Verification
Mutation-checked, each failing exactly one test and the right one:
TIMEOUTagainSKIP LOCKED, keepFOR UPDATERestored: 17 passed.
The lock assertion failed on its first run, and that is the reason to trust this approach. The
default SQLAlchemy dialect renders
FOR UPDATEand silently dropsSKIP LOCKED— it is aPostgreSQL extension. An assertion on
FOR UPDATEalone would have passed against a renderingproduction never gets: green for the wrong reason. The check now compiles against
postgresql.dialect(), and the helper's docstring records why.What the compiled-statement check proves, and what it does not. It proves the statement carries
FOR UPDATE SKIP LOCKEDand a SQL-side age comparison. It proves nothing about how PostgreSQLplans the query or whether an index is used — that needs a live database, which is outside what
this repo's tooling runs from here. Stated rather than implied.
A behavioural test was missing and I added it. Every other AC4 assertion passes while the sweep
writes whatever it likes, because the enum member existing is not the sweep using it.
_async_sweepis now driven through a fake session, which is also AC6's status half.On the
llc/suite being red locally — measured, not assumed. A fullllc/run gives 15failed / 66 errors. I reverted my three files to
origin/mainin place and re-ran the failingsubset both ways:
Identical, so the failures are pre-existing and environmental — consistent with the dependency
floor report stating 91 of 139 comparable packages sit below their declared floor in this
interpreter. CI is the authority; that local baseline carries no information about it.
black,isort,flake8clean. No file near the 600-line ceiling.Single-issue rationale
#16817 is v0.9.0 and its remaining ACs belong to #16818, which is a separate change to a different
concern (workspace lease modelling). Batching them would put an unreviewed release path into a PR
whose subject is detection.
Model Used
Claude Opus 5 (1M context)
Refs #16817, #16818
Summary by CodeRabbit