Repository navigation
Conversation
…ches (#15524) `asyncio.create_task()` is held by the loop only by weak reference, so a call whose return value is discarded can be collected before its coroutine runs — the defect diagnosed live in #15522, where one firing executed end to end and an identical firing minutes later produced no executor call, no inventory file and no log write, while the surface reported success both times. #15522 converted the three `_ansible_self_update` firings. This converts the five that remained in `autobot-slm-backend/`: api/infrastructure.py:494 playbook execution api/setup_wizard.py:1258 fleet provisioning api/updates.py:645 update discovery slm/agent/agent.py:493 code-change notification (both vendored copies) Each goes through `autobot_shared.async_compat.fire_and_forget`, which holds a hard reference until completion and logs a failure that otherwise has nowhere to surface — nothing awaits these tasks. The census in `repo_tests/background_task_retention_ratchet_test.py` is exact and shrink-only, so the five rows come out in the same commit. That empties the root, and the guard requires every scan root to be either censused or a proven zero, so `autobot-slm-backend/` moves into `PROVEN_ZERO_ROOTS` — where the zero is re-measured on the tree every run rather than recorded as finished. The two vendored copies of `slm/agent/agent.py` stay byte-identical. Closes #15524
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughFive discarded ChangesBackground task retention
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The background work remains scheduled and retained, with failures logged and the retention census still enforcing coverage. No merge-blocking risk is established. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
isort failed `code-quality` on autobot-slm-backend/api/setup_wizard.py: the fire_and_forget import was placed above `from api.websocket`, but the file's first-party block sorts api < autobot_shared < config. The other three converted files were already right — infrastructure.py has autobot_shared before services, updates.py puts async_compat before time_utils, and agent.py keeps the plain `from autobot_shared import ...` ahead of the submodule import. Checked rather than assumed, since the same edit pattern was applied to all four.
…ilings (#15524) `code-quality` failed: all five converted files sat exactly 4 lines over their recorded ceilings. Grandfathered files may not grow (#14236) — the exemption freezes the size it was granted for — so the change had to fit rather than the ceilings move. The 4 was structural, not incidental: one import line, plus a 1-line `asyncio.create_task(...)` becoming a 4-line `fire_and_forget(...)`. Each file is now net zero, and each by a different route because the files differ: agent.py (both copies) the existing `from autobot_shared import env_utils, time_utils` absorbs `async_compat` on the same line, so the import costs nothing; the call is one line. updates.py same trick, reached by switching `from autobot_shared.time_utils import utc_timestamp` to the module form and qualifying its four call sites. infrastructure.py the `# Start execution in background` comment above the call is what `fire_and_forget` now says in its own name. Removing it pays for the import. setup_wizard.py a four-line `logger.info(...)` that fits on one at 82 characters — its magic trailing comma was what kept Black from joining it. setup_wizard.py lands 2 UNDER, so its ceiling drops 1354 -> 1352 in both twin registries. A ratchet that can move down should. What the compaction cost, stated rather than buried: `name=` is now the bare identifier (`execution_id`, `job_id`, `commit[:12]`) instead of a prefixed label like `infrastructure-playbook:{execution_id}`. The prefixed form pushed the single line past 120 characters. The identifier alone is still unique and traceable in the failure log, which is what the name is for; the prefix was readability, and it was not worth a ceiling raise. Both vendored copies of agent.py stay byte-identical (`diff -q`, clean), and no line in any of the five exceeds 120 characters.
|
Carried by vehicle #17086, which includes this PR's approved head |
1 participant
Thinking Path
asyncio.create_task()is held by the event loop only by weak reference, so a call whosereturn value is discarded can be collected before its coroutine ever runs. #15522 diagnosed this
live: one firing executed end to end, an identical firing minutes later produced no executor call,
no inventory file and no log write — and the surface reported success both times, because nothing
awaits these tasks, so a raise inside the coroutine has nowhere to surface either.
#15524 enumerated 12 remaining sites. Measuring against merged
mainfirst rather than trustingthe issue's table, 7 were already gone:
api/stateful.py(3) went throughfire_and_forget,services/blue_green.py(1) took a retained assignment, andmonitoring/performance_dashboard.py(1) and
services/deployment.py(2) no longer contain the call at all — those two left by fileremoval/restructure rather than by conversion, which is worth stating plainly rather than ticking
as converted work.
The census constant has also drifted from the issue's text: #15619 and #15637 renamed
KNOWN_DISCARDED_CREATE_TASKtoKNOWN_DISCARDED_LAUNCHESand widened it from one root to five.The
autobot-backend/population it picked up is tracked separately by #15635, so #15524's{}criterion means theautobot-slm-backend/rows — which this PR empties.What Changed
Five discarded launches converted to
autobot_shared.async_compat.fire_and_forget, which holds ahard reference until completion and logs a failure that otherwise vanishes:
autobot-slm-backend/api/infrastructure.pyautobot-slm-backend/api/setup_wizard.pyautobot-slm-backend/api/updates.pyautobot-slm-backend/slm/agent/agent.pyautobot-slm-backend/ansible/roles/slm_agent/files/slm/agent/agent.pyEach gets a
name=carrying the job or execution id, so a failure names the work that was lost.repo_tests/background_task_retention_ratchet_test.py: the five rows come out ofKNOWN_DISCARDED_LAUNCHESin the same commit, as the shrink-only contract requires. That emptiesthe root, and
test_every_scan_root_is_either_censused_or_a_proven_zerorequires every scan rootto be one or the other — so
autobot-slm-backend/moves intoPROVEN_ZERO_ROOTS, wheretest_a_proven_zero_root_really_is_zerore-measures the claim on the tree every run instead oftaking the entry's word. The module docstring is corrected to match.
Verification
create_task/ensure_future/run_coroutine_threadsafecall inautobot-slm-backend/outside tests: 20 calls, all assignedor returned, 0 discarded. Checked independently of the AST detector, and the two agree.
that dies at return, which the detector's bare-expression rule cannot see. Every one of the 11
plausible cases resolves to a real holder;
main.py:283is the tightest and stores into_code_sync_mod._running_tasks[_job_id]on the next line.diff -q, clean) — AC3.asynciostill used in all four edited files (3/7/5/2 references), so no import is orphaned.ast.parse).FIX THE SWEEPassertions (AC4) are CI's to prove; they are notclaimed here.
Not run locally: this repository's code is never executed from the checkout — CI is the evidence.
Model Used
Claude Opus 5
Closes #15524
Single-issue rationale
#15524 cannot ride with another issue because its ratchet contract forbids it. The census in
repo_tests/background_task_retention_ratchet_test.pyis exact and shrink-only: every conversionmust lower the census in the same commit, and emptying the root additionally moves it into
PROVEN_ZERO_ROOTS. A second issue's changes in the same PR would make a failure of that guardambiguous between the two.
The obvious batching partner is #15635, which owns the remaining
autobot-backend/census. It isexplicitly not ridden with: it is blocked on decomposing 19 grandfathered files, a different and far
larger piece of work at a different risk level.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests