fix(cleanup): force-fail orphan when agent unreachable during re-verify (#497) by dolho · Pull Request #783 · Abilityai/trinity

dolho · 2026-05-11T12:45:40Z

Summary

The Phase 3 re-verify branch in _process_stale_slot_reclaims (added by #378) treated "agent unreachable" the same as "still running" — it continue-d. The Redis slot was already reclaimed by capacity.reclaim_stale() (TTL elapsed), but the DB execution row stayed in running until Phase 1's 120-min stale cleanup picked it up. Under sustained 3/3-capacity load with intermittent agent unreachability, this left zombie running rows for up to 2 hours.

After this PR, the unreachable branch calls db.fail_stale_slot_execution immediately — the same race-guarded writer already used by the "re-verify-confirmed-inactive" branch. The slot was reclaimed because TTL elapsed (timeout_seconds + buffer), so by construction the execution is already older than Phase 1's threshold at a much longer window.

Related to #497.

Investigation

The issue noted: "validation needed during implementation — root cause framing inferred from cleanup logs and DB-vs-slots divergence on a live instance. Not yet reproduced in isolation."

Done:

1. Traced the SUCCESS-write paths and race guards

Writer	Guard	Wins over
`update_execution_status(SUCCESS)` (`db/schedules.py:1048`)	`WHERE id=? AND status != 'cancelled'`	FAILED, RUNNING, anything except CANCELLED
`update_execution_status(FAILED)`	`WHERE id=? AND status NOT IN (success,failed,cancelled,skipped)`	only RUNNING/QUEUED
`fail_stale_slot_execution` (`db/schedules.py:1615`)	`WHERE id=? AND status='running'`	only RUNNING
`mark_execution_failed_by_watchdog`	similar guard	only RUNNING

The fail_stale_slot_execution race guard is the right primitive: it preserves any SUCCESS / FAILED / CANCELLED that landed between the slot reclaim and the cleanup write.

2. Confirmed the slot "leak" framing was imprecise

slot_service._cleanup_stale_slots_for_agent does zremrangebyscore (slot_service.py:301) — reclaimed slots are removed from Redis permanently. Capacity is Redis-based (zcard), so reclaimed slots do not hold capacity. The "Agent at capacity 3/3" the reporter saw must be 3 legitimate live tasks. The actual symptom is a DB-row zombie that lingers for up to 120 min, polluting dashboards and operator-queue visibility.

3. Built an isolated repro

tests/unit/test_cleanup_unreachable_orphan.py drives the exact decision matrix:

✅ test_agent_unreachable_force_fails_orphan — unreachable → race-guarded fail with error tag
✅ test_agent_reachable_says_still_running_no_action — bug: Execution shows Failed then Success after cleanup (race condition) #378 path preserved
✅ test_agent_reachable_says_not_running_fails — existing behavior preserved
✅ test_confirmed_running_skipped_even_on_unreachable — Phase 0's confirmed_running_ids still wins
✅ test_race_guard_no_increment_when_db_says_already_terminal — counter respects the WHERE guard
✅ test_mixed_batch_some_unreachable_some_confirmed — per-execution decision matrix routing

Documented residual risk

If the agent later recovers and writes SUCCESS via update_execution_status, that path overwrites FAILED per #378's "SUCCESS wins over FAILED" rule. The execution must have run past timeout + buffer for its slot to be reclaimed in the first place, so a "late SUCCESS" represents a deliverable that exceeded its budget. Accepted as a sharp narrowing of the 2-hour-zombie window.

Follow-up direction if it ever becomes a real complaint: narrow update_execution_status's SUCCESS-over-FAILED rule to exclude FAILED rows tagged with error LIKE 'Stale execution — agent % unresponsive%'. Mechanically straightforward, but expands behavior beyond this issue and was not bundled here.

What stays unchanged

The confirmed_running_ids shortcut from Phase 0 (bug: Stale slot cleanup uses fixed 20-min TTL regardless of agent timeout #226).
The "re-verify says still running" branch (bug: Execution shows Failed then Success after cleanup (race condition) #378 fix).
The "re-verify says inactive" failure path.
Phase 1's 120-min stale cleanup as the safety net for executions whose slots were NOT in reclaimed (still alive in Redis).

Test plan

6 new unit tests pass — all six pin a distinct case
Two pre-existing watchdog tests updated (test_skips_when_agent_unreachable → test_force_fails_when_agent_unreachable; test_one_agent_raises_others_proceed now asserts both execs failed) and pass
Joint run: 41 passed (test_watchdog_unit.py + test_cleanup_unreachable_orphan.py + test_session_cold_turn_lock.py)
No regression: test_watchdog_unit.py collection error in isolation is pre-existing — docker py-pkg missing from the unit test venv, reproduces on origin/dev via git stash (unrelated to this PR)
Reviewer pass

Files

File	Change
`src/backend/services/cleanup_service.py`	Replace `continue` on unreachable branch with race-guarded `fail_stale_slot_execution`. ~40 lines incl. updated docstring.
`tests/test_watchdog_unit.py`	Two existing pins flipped to reflect new behavior.
`tests/unit/test_cleanup_unreachable_orphan.py` (new)	6-test suite covering the full decision matrix.

🤖 Generated with Claude Code

…fy (#497) Before ------ `_process_stale_slot_reclaims`' Phase 3 re-verify branch (added by #378 to prevent FAILED→SUCCESS flicker) treated "agent unreachable" the same as "still running" — it `continue`-d the loop. The slot was already reclaimed from Redis by `capacity.reclaim_stale()` (TTL elapsed), but the DB execution row stayed in `running` until Phase 1's 120-min stale cleanup picked it up. Under sustained 3/3-capacity load with intermittent agent unreachability (CPU-pinned, network-stuck, container hung), this leaves zombie `running` rows for up to 2 hours, polluting dashboards and the operator-queue's view of fleet state. After ----- When `_get_agent_running_ids(agent)` returns None (unreachable), `_process_stale_slot_reclaims` now calls `db.fail_stale_slot_execution` — the same race-guarded writer already used for the "re-verify-confirmed-inactive" branch. The slot was reclaimed by TTL, so the execution is by construction older than `timeout + buffer`; Phase 1's 120-min wait is the same condition at a much longer window. We can apply it immediately and free the row. Race safety ----------- `fail_stale_slot_execution` is guarded by `WHERE status='running'`, so a SUCCESS / FAILED / CANCELLED that landed between the slot reclaim and this cleanup write is preserved (race-guard returns False, counter does not increment, cleanup log records a benign debug line). Documented residual flicker risk -------------------------------- If the agent later recovers and writes SUCCESS via `update_execution_status`, that path overwrites FAILED per #378's "SUCCESS wins" rule. The execution must have run past its configured `timeout + buffer` for the slot to be reclaimed in the first place, so a "late SUCCESS" represents a deliverable that exceeded its budget. This is documented behavior, accepted as a sharp narrowing of the 2-hour-zombie window. If it becomes a real operator complaint, follow-up work would narrow `update_execution_status`'s SUCCESS-over-FAILED rule to exclude cleanup-induced markers — see PR body for the proposed design. Investigation ------------- The issue (#497) noted "validation needed during implementation — root cause framing inferred from cleanup logs and DB-vs-slots divergence on a live instance. Not yet reproduced in isolation." Done now: - Traced every `status='success'` write path and its race guard (db/schedules.py:update_execution_status `WHERE status != 'cancelled'` is the relevant overwrite rule). - Confirmed that slot leakage per se does NOT happen — slot is removed from Redis ZSET permanently in `_cleanup_stale_slots_for_agent`. The symptom is DB-row zombie, not slot zombie. Capacity rejection under load is from real live tasks, not the orphan row. - Built a 6-test unit suite that drives the exact decision matrix. Tests ----- - `tests/unit/test_cleanup_unreachable_orphan.py` (new, 6 tests): - unreachable → force-fail with race-guarded writer + error tag - reachable + still-running → no action (#378 path) - reachable + not-running → fail (existing behavior) - confirmed_running set wins over unreachable - race-guard returns False → counter not incremented - mixed-batch routing: unreachable vs confirmed across agents - `tests/test_watchdog_unit.py`: two existing tests pinned the OLD defer-on-unreachable behavior — updated to assert the new force-fail semantics + the error-tag invariant. Related to #497. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Surfaced by /validate-pr — behavior change in `_process_stale_slot_reclaims` warranted a feature-flow update. Updates the Phase 3 per-execution decision matrix (unreachable → FAIL now instead of SKIP), notes the documented #497 residual flicker risk, and adds a row to the Error Handling table separating Phase 0 vs Phase 3 unreachable semantics. Related to #497. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

dolho · 2026-05-11T13:29:27Z

PR Validation Report (manual run of `/validate-pr` checklist)

Note: the /validate-pr skill lives in the .claude private submodule (extracted by #774). Submodule was uninitialized locally; cloned via SSH override mid-session. Harness caches skill list at session start, so the skill itself wasn't invocable — ran the checklist manually.

Category	Status	Notes
Issue link	✅	"Related to #497" — non-closing per policy (issue stays open till merge to main)
Commit Messages	✅	2 commits, conventional format (`fix(cleanup):`, `docs(cleanup-service):`)
Base Branch	✅	Targets `dev`
PR Size	✅	4 files (after doc update), well under 50
Issue priority	✅	P2 / `type-bug`
Requirements	➖	Not applicable — bug fix to existing service, no new capability
Architecture	➖	No API/schema/component change; Cleanup Service entry in architecture.md still accurate (the `_run_cleanup_inner` description doesn't pin Phase 3 decision matrix at that level of detail)
Feature Flow	✅	`docs/memory/feature-flows/cleanup-service.md` updated (commit `62cbb29c`) — Phase 3 decision matrix flipped from SKIP to FAIL on unreachable, residual #497 flicker risk documented, Error Handling table split into Phase 0 vs Phase 3 unreachable rows
Security Check	✅	No API keys, real emails, public IPs, .env files, hardcoded secrets, or credential files in the diff
Code Quality	✅	Reuses existing primitives: `fail_stale_slot_execution`'s `WHERE status='running'` race-guard, the same per-execution try/except shape as the sibling "re-verify-says-inactive" branch. Per-execution error isolation preserved.
Requirements Trace	✅	Issue #497 explicitly referenced; investigation comment in PR body addresses the issue's "validation needed during implementation" flag
Unit tests	✅	6 new tests in `tests/unit/test_cleanup_unreachable_orphan.py` all pass; 2 pre-existing `test_watchdog_unit.py` tests flipped to match new behavior; joint 41-test run green

Issues Found

Critical: none.

Warnings (resolved during validation):

~~Feature flow cleanup-service.md not updated~~ — pushed in 62cbb29c, fixes the Phase 3 decision matrix to reflect FAIL-on-unreachable, adds the residual-risk note, and splits the Error Handling row.

Suggestions (non-blocking):

No live-stack soak repro was attempted. Issue's reproduction recipe (3/3 capacity + induced uvicorn worker disconnect + sustained agent CPU pin) requires real load and a real kill -9 on a worker. Unit-level repro and decision-matrix tests cover the logic correctness; the production scenario is the soak gap. Worth flagging for a follow-up "verify on real fleet" task once this lands.
The documented residual flicker (late SUCCESS overwrites cleanup-induced FAILED via the existing bug: Execution shows Failed then Success after cleanup (race condition) #378 rule) is real but bounded. If it ever bites in production, the follow-up direction is narrowing update_execution_status's SUCCESS-over-FAILED rule to exclude FAILED rows tagged with the cleanup marker.

Recommendation

APPROVE — ready for human review + merge to dev.

PR addresses the specific behavior the issue complained about: cleanup tick now immediately fails the orphan via the race-guarded writer instead of waiting Phase 1's 120-min stale-cleanup window. Race safety preserved via the existing WHERE status='running' guard. #378 paths (re-verify confirms still-running, confirmed_running shortcut, sibling decision-matrix branches) unchanged. Documented residual flicker risk is accepted; follow-up direction included in PR body.

🤖 Generated with Claude Code

…ssage The lint at `tests/lint_sys_modules.py` documents three ways out of a bare-`sys.modules` violation: `monkeypatch.setitem` / `delitem`, moving to `conftest.py`, and the `_STUBBED_MODULE_NAMES` + `_restore_sys_modules` named-helper pair (precedent: `tests/unit/test_telegram_webhook_backfill.py`). Only the first two are surfaced in the per-line error message. For tests that load real backend modules in isolation with their heavy deps stubbed at import time, neither of the documented options applies: - `monkeypatch.*` is fixture-scoped and structurally cannot rewind a stub planted before any test runs (the stubs are needed during `spec_from_file_location` + `exec_module`, which executes at module-import time). - `conftest.py` would leak the stubs to every test in the directory, the exact #762 failure mode the lint exists to prevent. The named-helper escape hatch is the right answer for this case, and the lint already exempts it — but discoverability is poor. PR #783 and #606 both hit the lint, read the error, and re-discovered the escape hatch independently after digging into the script's docstring. Surface it in the error directly so the next author sees all three options at first encounter, not after archaeology. No behavioral change — pure error-text expansion. The lint's exit codes, detection logic, and baseline mechanics are unchanged. The committed baseline file is unaffected. Refs #762 Co-Authored-By: Claude <noreply@anthropic.com>

`tests/unit/test_cleanup_unreachable_orphan.py` (added in 79a6481, PR #783) plants three import-time stubs in `sys.modules`: - `docker` (line 64) — the unit venv doesn't ship the docker package - `services.docker_service` (line 76) — eager-imported by `services/__init__.py` - `database` (line 79) — hit transitively by cleanup_service The lint+baseline shipped in #791 (3043631) merged ~4 hours after #783, but `#791`'s baseline file omits this entry — almost certainly generated against a pre-#783 dev state and never refreshed before merge. Result: `lint-sys-modules` job in `backend-unit-test.yml` has been red on dev since both PRs landed, and the failure travels to every PR branched off that state — including #606's PR. Apply the same named-helper escape hatch used by the #606 file and the original `tests/unit/test_telegram_webhook_backfill.py` precedent: a top-level `_STUBBED_MODULE_NAMES` list naming the three slots, plus an autouse `_restore_sys_modules` fixture that snapshots them at test setup and restores at teardown. Bounds the cross-file pollution to this test's own scope — the exact invariant #762's lint protects. Verification: - `python tests/lint_sys_modules.py` — clean: 223 violations in 65 files, baseline allows 223, no new violations. - `pytest tests/test_lint_sys_modules.py` — all 25 tests pass, including `test_committed_baseline_matches_current_repo_state` (was failing). - `pytest tests/unit/test_cleanup_unreachable_orphan.py` — passes. Test-only — no production code changes. Cross-PR fix: clears CI on this branch and on dev. The underlying "stale baseline at PR merge" question (why didn't #791's own CI catch this?) is a separate concern worth a follow-up issue. Refs #762 Refs #783 Co-Authored-By: Claude <noreply@anthropic.com>

…ssage The lint at `tests/lint_sys_modules.py` documents three ways out of a bare-`sys.modules` violation: `monkeypatch.setitem` / `delitem`, moving to `conftest.py`, and the `_STUBBED_MODULE_NAMES` + `_restore_sys_modules` named-helper pair (precedent: `tests/unit/test_telegram_webhook_backfill.py`). Only the first two are surfaced in the per-line error message. For tests that load real backend modules in isolation with their heavy deps stubbed at import time, neither of the documented options applies: - `monkeypatch.*` is fixture-scoped and structurally cannot rewind a stub planted before any test runs (the stubs are needed during `spec_from_file_location` + `exec_module`, which executes at module-import time). - `conftest.py` would leak the stubs to every test in the directory, the exact #762 failure mode the lint exists to prevent. The named-helper escape hatch is the right answer for this case, and the lint already exempts it — but discoverability is poor. PR #783 and #606 both hit the lint, read the error, and re-discovered the escape hatch independently after digging into the script's docstring. Surface it in the error directly so the next author sees all three options at first encounter, not after archaeology. No behavioral change — pure error-text expansion. The lint's exit codes, detection logic, and baseline mechanics are unchanged. The committed baseline file is unaffected. Refs #762 Co-Authored-By: Claude <noreply@anthropic.com>

`tests/unit/test_cleanup_unreachable_orphan.py` (added in 79a6481, PR #783) plants three import-time stubs in `sys.modules`: - `docker` (line 64) — the unit venv doesn't ship the docker package - `services.docker_service` (line 76) — eager-imported by `services/__init__.py` - `database` (line 79) — hit transitively by cleanup_service The lint+baseline shipped in #791 (3043631) merged ~4 hours after #783, but `#791`'s baseline file omits this entry — almost certainly generated against a pre-#783 dev state and never refreshed before merge. Result: `lint-sys-modules` job in `backend-unit-test.yml` has been red on dev since both PRs landed, and the failure travels to every PR branched off that state — including #606's PR. Apply the same named-helper escape hatch used by the #606 file and the original `tests/unit/test_telegram_webhook_backfill.py` precedent: a top-level `_STUBBED_MODULE_NAMES` list naming the three slots, plus an autouse `_restore_sys_modules` fixture that snapshots them at test setup and restores at teardown. Bounds the cross-file pollution to this test's own scope — the exact invariant #762's lint protects. Verification: - `python tests/lint_sys_modules.py` — clean: 223 violations in 65 files, baseline allows 223, no new violations. - `pytest tests/test_lint_sys_modules.py` — all 25 tests pass, including `test_committed_baseline_matches_current_repo_state` (was failing). - `pytest tests/unit/test_cleanup_unreachable_orphan.py` — passes. Test-only — no production code changes. Cross-PR fix: clears CI on this branch and on dev. The underlying "stale baseline at PR merge" question (why didn't #791's own CI catch this?) is a separate concern worth a follow-up issue. Refs #762 Refs #783 Co-Authored-By: Claude <noreply@anthropic.com>

…leanup_unreachable_orphan.py (#796) #791 (which added the tests/lint_sys_modules.py lint and its baseline) landed AFTER #783 (which added tests/unit/test_cleanup_unreachable_orphan.py). The baseline was generated without including the orphan-test file's 3 top-level `sys.modules[...] = stub` assignments at lines 64, 76, 79 (docker, services.docker_service, database stubs installed before the test module's services.cleanup_service import). Result: every PR opened against dev fails the lint job on these 3 lines, even when the PR's own changes are clean. Fix: grandfather the file into the baseline at its current count (3). This matches how the same shape is handled elsewhere — e.g. test_watchdog_unit.py (3 violations, the precedent the orphan-test file's own comment cites). The lint script's baseline path is the intended mechanism for accepting pre-existing module-level stub installs that cannot use monkeypatch. Verified locally: python3 tests/lint_sys_modules.py emits zero violations in the real test tree (only .venv site-packages remain, which CI runners don't have). Refs #762, #783, #791 Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…form_default_model The lint job on #443's PR went red on a file that is NOT in the PR's diff — `tests/test_platform_default_model.py` carries 6 bare `sys.modules` mutations that were merged to dev without an entry in `tests/lint_sys_modules_baseline.txt`. Same #802 rebase-race shape as the original #791 vs #783 incident: baseline-introducing PR (#791) and violation-introducing PR landed disjointly, both green pre-merge, dev went red post-merge, the next PR off dev (this one) inherits it. Ratcheting the baseline here unblocks #443. Same precedent as #796 which baselined `test_cleanup_unreachable_orphan.py` for the same reason. The proper fix is to convert the file's bare sys.modules writes to `monkeypatch.setitem` / `monkeypatch.delitem`; that's scope-creep relative to the onboarding work this PR is doing and should land separately. (That file's count is currently 0 against the lint, so a follow-up that converts it can drop the baseline line altogether.) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…race (#828) Issue #802: baseline-style lint (sys.modules pollution check) only ran on pull_request, so two PRs disjointly touching baseline + new test file could both pass pre-merge CI yet leave dev red post-merge. Concrete case: #791 landed the lint baseline 4h after CI green; meanwhile #783 merged a new test file with 3 baselined-elsewhere violations. No re-run on either side caught the resulting state — the next PR off dev (#606) inherited a red CI. Adds a push trigger on dev/main so lint-sys-modules fires post-merge. Per-PR diff jobs (test, diff) are gated to pull_request only since base/head don't exist on a push. Related to #802 Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(onboarding): six defects in self-host deploy path (#443) Friction surfaced on a fresh clone → start.sh → login walkthrough. Each fix is small and independent; bundled here per the issue's "all are onboarding hygiene" framing. 1. scripts/deploy/start.sh — auto-generate SECRET_KEY and INTERNAL_API_SECRET if blank (same pattern as the existing CREDENTIAL_ENCRYPTION_KEY block, extracted to a helper). Fail fast with a clear message if ADMIN_PASSWORD is blank rather than booting into a state the operator can't log into. 2. .env.example — document FRONTEND_PORT=80 with an inline comment explaining "remap if your host's port 80 is taken." The var was already read by docker-compose.yml and start.sh but wasn't reachable from the quickstart. 3. scripts/deploy/start.sh — replace "login: admin/password" hint with "login: admin / ADMIN_PASSWORD from .env". New users took "password" as the literal default. 4. src/mcp-server/src/index.ts + README.md — drop hardcoded http://localhost:3000/api-keys hint. Port was wrong (frontend default is 80) and ignored FRONTEND_PORT overrides. Replaced with "your Trinity web UI → Settings → MCP Keys". 5. scripts/deploy/clean.sh — new script. Stops compose + removes leftover agent-* containers + trinity-agent-network. Without this, a fresh install inherits Exited zombie agents from previous tests and the first new agent collides on AGENT_SSH_PORT_START (2222). Preserves data volumes intentionally — operators can wipe those manually. 6. src/mcp-server/Dockerfile — point healthcheck at /health, not /mcp. The /mcp endpoint rejects HEAD requests with 400 (which `wget --spider` sends), so `docker compose ps` reported the container as unhealthy despite serving traffic. /health returns 200 to both HEAD and GET. Verified live: post-rebuild, trinity-mcp-server reports `healthy` within 30s. No schema, API, or runtime-behavior changes. No migration needed. Related to #443 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(tests): baseline 6 pre-existing sys.modules writes in test_platform_default_model The lint job on #443's PR went red on a file that is NOT in the PR's diff — `tests/test_platform_default_model.py` carries 6 bare `sys.modules` mutations that were merged to dev without an entry in `tests/lint_sys_modules_baseline.txt`. Same #802 rebase-race shape as the original #791 vs #783 incident: baseline-introducing PR (#791) and violation-introducing PR landed disjointly, both green pre-merge, dev went red post-merge, the next PR off dev (this one) inherits it. Ratcheting the baseline here unblocks #443. Same precedent as #796 which baselined `test_cleanup_unreachable_orphan.py` for the same reason. The proper fix is to convert the file's bare sys.modules writes to `monkeypatch.setitem` / `monkeypatch.delitem`; that's scope-creep relative to the onboarding work this PR is doing and should land separately. (That file's count is currently 0 against the lint, so a follow-up that converts it can drop the baseline line altogether.) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* chore(tests): baseline 3 pre-existing sys.modules mutations in test_cleanup_unreachable_orphan.py #791 (which added the tests/lint_sys_modules.py lint and its baseline) landed AFTER #783 (which added tests/unit/test_cleanup_unreachable_orphan.py). The baseline was generated without including the orphan-test file's 3 top-level `sys.modules[...] = stub` assignments at lines 64, 76, 79 (docker, services.docker_service, database stubs installed before the test module's services.cleanup_service import). Result: every PR opened against dev fails the lint job on these 3 lines, even when the PR's own changes are clean. Fix: grandfather the file into the baseline at its current count (3). This matches how the same shape is handled elsewhere — e.g. test_watchdog_unit.py (3 violations, the precedent the orphan-test file's own comment cites). The lint script's baseline path is the intended mechanism for accepting pre-existing module-level stub installs that cannot use monkeypatch. Verified locally: python3 tests/lint_sys_modules.py emits zero violations in the real test tree (only .venv site-packages remain, which CI runners don't have). Refs #762, #783, #791 Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tests): override "test" sentinel in security conftest (closes #804) `tests/conftest.py` setdefaults REDIS_PASSWORD/REDIS_BACKEND_PASSWORD to the literal "test" at global pytest import so backend modules can be imported without real Redis creds. `tests/security/conftest.py` was then using `os.environ.setdefault(...)` to overlay real `.env` values — a no-op because the sentinel was already set. Result: `redis-cli` ran with `-a test` against a healthy stack and the ACL acceptance tests failed for the wrong reason. Pop the sentinel before the `.env` overlay and use direct assignment. Add a regression test that asserts the live env value is not the sentinel and (when `.env` defines the keys) matches the `.env` value. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…ssage The lint at `tests/lint_sys_modules.py` documents three ways out of a bare-`sys.modules` violation: `monkeypatch.setitem` / `delitem`, moving to `conftest.py`, and the `_STUBBED_MODULE_NAMES` + `_restore_sys_modules` named-helper pair (precedent: `tests/unit/test_telegram_webhook_backfill.py`). Only the first two are surfaced in the per-line error message. For tests that load real backend modules in isolation with their heavy deps stubbed at import time, neither of the documented options applies: - `monkeypatch.*` is fixture-scoped and structurally cannot rewind a stub planted before any test runs (the stubs are needed during `spec_from_file_location` + `exec_module`, which executes at module-import time). - `conftest.py` would leak the stubs to every test in the directory, the exact #762 failure mode the lint exists to prevent. The named-helper escape hatch is the right answer for this case, and the lint already exempts it — but discoverability is poor. PR #783 and #606 both hit the lint, read the error, and re-discovered the escape hatch independently after digging into the script's docstring. Surface it in the error directly so the next author sees all three options at first encounter, not after archaeology. No behavioral change — pure error-text expansion. The lint's exit codes, detection logic, and baseline mechanics are unchanged. The committed baseline file is unaffected. Refs #762 Co-Authored-By: Claude <noreply@anthropic.com>

`tests/unit/test_cleanup_unreachable_orphan.py` (added in 79a6481, PR #783) plants three import-time stubs in `sys.modules`: - `docker` (line 64) — the unit venv doesn't ship the docker package - `services.docker_service` (line 76) — eager-imported by `services/__init__.py` - `database` (line 79) — hit transitively by cleanup_service The lint+baseline shipped in #791 (3043631) merged ~4 hours after #783, but `#791`'s baseline file omits this entry — almost certainly generated against a pre-#783 dev state and never refreshed before merge. Result: `lint-sys-modules` job in `backend-unit-test.yml` has been red on dev since both PRs landed, and the failure travels to every PR branched off that state — including #606's PR. Apply the same named-helper escape hatch used by the #606 file and the original `tests/unit/test_telegram_webhook_backfill.py` precedent: a top-level `_STUBBED_MODULE_NAMES` list naming the three slots, plus an autouse `_restore_sys_modules` fixture that snapshots them at test setup and restores at teardown. Bounds the cross-file pollution to this test's own scope — the exact invariant #762's lint protects. Verification: - `python tests/lint_sys_modules.py` — clean: 223 violations in 65 files, baseline allows 223, no new violations. - `pytest tests/test_lint_sys_modules.py` — all 25 tests pass, including `test_committed_baseline_matches_current_repo_state` (was failing). - `pytest tests/unit/test_cleanup_unreachable_orphan.py` — passes. Test-only — no production code changes. Cross-PR fix: clears CI on this branch and on dev. The underlying "stale baseline at PR merge" question (why didn't #791's own CI catch this?) is a separate concern worth a follow-up issue. Refs #762 Refs #783 Co-Authored-By: Claude <noreply@anthropic.com>

…rt (#606) (#800) * test(subscription): pin auto-switch chain skips .credentials.enc import (#606) Adds a chain-level regression test for #606 entered at `subscription_auto_switch._restart_agent` — pins that the SUB-003 auto-switch path reaches the `lifecycle.py:155` `subscription_mode` short-circuit and never re-enters the file-based credential import. The existing leaf-level test (`test_inject_assigned_credentials.py::test_subscription_mode_skips_import_with_clear_reason`) covers the short-circuit in isolation but would not catch a future refactor of `_perform_auto_switch` or `_restart_agent` that quietly bypassed the guard. The new test asserts on `reason == "subscription_mode"` specifically (not just `status == "skipped"`) so a refactor that moved the short-circuit into the `#421` `container_already_running` branch would fail explicitly. Test-only — no production code changes. Refs #606 Co-Authored-By: Claude <noreply@anthropic.com> * docs(feature-flows): sync session-tab cold-turn lock + subscription auto-switch test session-tab.md: step 6 documents the new `_ResumeLock(..., session.id, ...)` signature and the cold-turn key shape `session_lock:cold:{session_id}` (#779). Step 5 drops the "which the lock skips by design" claim about cold turns — no longer accurate. subscription-auto-switch.md: adds `tests/unit/test_subscription_auto_switch_no_cred_import.py` to the Tests table — chain-level regression for #606 that pins the SUB-003 auto-switch path reaches the `lifecycle.py:155` subscription_mode short-circuit and never re-enters file-based credential import. Refs #779 Refs #606 Co-Authored-By: Claude <noreply@anthropic.com> * fix(tests): adopt sys.modules snapshot/restore in #606 chain test The #606 chain-level regression test added in 967b874 introduced 17 new `tests/lint_sys_modules.py` violations — `sys.modules[...] = ...`, `.update(...)`, `.pop(...)`, `.setdefault(...)` writes at both module- import time (inside `_preload_credential_encryption`, `_load_lifecycle`, `_load_auto_switch`) and inside the test body and `_reset` fixture. `monkeypatch.setitem` is fixture-scoped and structurally cannot rewind the stubs planted during module import. Adopt the named snapshot/restore escape hatch documented in `tests/lint_sys_modules.py` (precedent: `tests/unit/test_telegram_webhook_backfill.py`): - Add top-level `_STUBBED_MODULE_NAMES` listing the 19 module slots this file stubs across its four loader paths. - Add autouse `_restore_sys_modules` fixture that snapshots those slots at test setup and restores them at teardown. This bounds the actual #762 concern (cross-file pollution within the same pytest session) without forcing a structurally-impossible rewrite of the import-time loaders. Verification: - `python tests/lint_sys_modules.py` — target file now reports 0 violations (was 17). The remaining `test_cleanup_unreachable_orphan.py` failure is a pre-existing unrelated case. - `pytest tests/unit/test_subscription_auto_switch_no_cred_import.py` — passes. - `pytest tests/unit/test_inject_assigned_credentials.py` — passes, confirming the snapshot/restore doesn't leak into the sibling leaf test that also loads `lifecycle.py`. Test-only — no production code changes. Refs #606 Refs #762 Co-Authored-By: Claude <noreply@anthropic.com> * docs(lint): mention named-helper escape hatch in sys.modules error message The lint at `tests/lint_sys_modules.py` documents three ways out of a bare-`sys.modules` violation: `monkeypatch.setitem` / `delitem`, moving to `conftest.py`, and the `_STUBBED_MODULE_NAMES` + `_restore_sys_modules` named-helper pair (precedent: `tests/unit/test_telegram_webhook_backfill.py`). Only the first two are surfaced in the per-line error message. For tests that load real backend modules in isolation with their heavy deps stubbed at import time, neither of the documented options applies: - `monkeypatch.*` is fixture-scoped and structurally cannot rewind a stub planted before any test runs (the stubs are needed during `spec_from_file_location` + `exec_module`, which executes at module-import time). - `conftest.py` would leak the stubs to every test in the directory, the exact #762 failure mode the lint exists to prevent. The named-helper escape hatch is the right answer for this case, and the lint already exempts it — but discoverability is poor. PR #783 and #606 both hit the lint, read the error, and re-discovered the escape hatch independently after digging into the script's docstring. Surface it in the error directly so the next author sees all three options at first encounter, not after archaeology. No behavioral change — pure error-text expansion. The lint's exit codes, detection logic, and baseline mechanics are unchanged. The committed baseline file is unaffected. Refs #762 Co-Authored-By: Claude <noreply@anthropic.com> * fix(tests): adopt sys.modules snapshot/restore in #783 cleanup test `tests/unit/test_cleanup_unreachable_orphan.py` (added in 79a6481, PR #783) plants three import-time stubs in `sys.modules`: - `docker` (line 64) — the unit venv doesn't ship the docker package - `services.docker_service` (line 76) — eager-imported by `services/__init__.py` - `database` (line 79) — hit transitively by cleanup_service The lint+baseline shipped in #791 (3043631) merged ~4 hours after #783, but `#791`'s baseline file omits this entry — almost certainly generated against a pre-#783 dev state and never refreshed before merge. Result: `lint-sys-modules` job in `backend-unit-test.yml` has been red on dev since both PRs landed, and the failure travels to every PR branched off that state — including #606's PR. Apply the same named-helper escape hatch used by the #606 file and the original `tests/unit/test_telegram_webhook_backfill.py` precedent: a top-level `_STUBBED_MODULE_NAMES` list naming the three slots, plus an autouse `_restore_sys_modules` fixture that snapshots them at test setup and restores at teardown. Bounds the cross-file pollution to this test's own scope — the exact invariant #762's lint protects. Verification: - `python tests/lint_sys_modules.py` — clean: 223 violations in 65 files, baseline allows 223, no new violations. - `pytest tests/test_lint_sys_modules.py` — all 25 tests pass, including `test_committed_baseline_matches_current_repo_state` (was failing). - `pytest tests/unit/test_cleanup_unreachable_orphan.py` — passes. Test-only — no production code changes. Cross-PR fix: clears CI on this branch and on dev. The underlying "stale baseline at PR merge" question (why didn't #791's own CI catch this?) is a separate concern worth a follow-up issue. Refs #762 Refs #783 Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>

The first CI run this PR was ever able to produce (it was CONFLICTING until now, so GitHub could not build refs/pull/1827/merge and reported zero checks) failed `lint (sys.modules pollution check)`: 12 bare `sys.modules.pop(...)` calls, 3 per new file. Fixed by satisfying the guard, NOT by widening its baseline (`lint_sys_modules_baseline.txt` is untouched) — this guard has already caught this same class in #783, #606 and #875, and retiring it on a test-rigour PR would be backwards. Both eviction sites are out of monkeypatch's reach, or actively wrong for it: * the `utils*` shadow clear is IMPORT-time, before any fixture exists, so monkeypatch structurally cannot reach it; * the `ops` fixture must evict `db.*` so `from db.schedules import ...` re-imports against the harness-bound engine. `monkeypatch.delitem` records NO undo for a key absent on entry (verified against _pytest.monkeypatch), so the freshly imported harness-bound module would stay resident for later files — strictly worse isolation than the explicit pop it would replace. So both take the documented escape hatch instead: a top-level `_STUBBED_MODULE_NAMES` list plus an autouse `_restore_sys_modules` fixture, matching the shape of tests/unit/test_telegram_webhook_backfill.py. It is a teardown-time guarantee only — the snapshot is taken before the test body, so no in-test behaviour and no assertion changes. Verified: lint reports "203 violation(s) in 60 file(s); baseline allows 240 — no new violations"; the four files still report exactly 129 passed, 1 xfailed (same strict xfail); `git diff origin/dev...HEAD -- src/` still empty. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…ers and analytics aggregation (#1827) * test(db/schedules): edge cases + Hypothesis properties for CAS status writers (#1771) /edge-cases target 3, sub-area A: the #1082 status-as-projection CAS writers in db/schedules/executions.py and db/schedules/queue.py. - test_1771c_schedules_cas_edges.py: matrix rows A1b/A2/A3/A5/A5b/A6/A7/A8/ A12/A13/A14/A15/A16 as parametrized cases on the db_harness (#300) full production schema — SQLite always, PostgreSQL when TEST_POSTGRES_URL is set. - test_1771c_schedules_cas_properties.py: P-A1 bounded RuleBasedStateMachine ("terminal is absorbing" — canary E-02 at the DB layer) plus a meta-test that injects a phantom reversal and proves the machine reports it; P-A2/P-A3 no-crash + idempotence asserted PER RETURN CLASS (bool / int / Optional[Dict] are three different contracts); P-A4 lexicographic-ISO oracle. - hypothesis==6.161.5 added to tests/requirements-test.txt (exact pin so the three concurrent #1771 slices produce an identical, trivially-mergeable line). One strict xfail: A6 — a clock-skewed started_at persists a NEGATIVE duration_ms. No product-code fix (#1771 AC#4). Test-only. No src/ changes. Refs #1771 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(db/schedules): edge cases + Hypothesis properties for analytics aggregation (#1771) /edge-cases target 3, sub-area B: db/schedules/analytics.py — get_agent_analytics (#1107), get_schedule_analytics (#868), get_agent_schedules_summary (#1115) and the pure leaves _bucket_for_trigger / _schedule_command_label. - test_1771c_schedules_analytics_edges.py: matrix rows B1–B12. Highlights: B1 pins _TRIGGER_BUCKETS.values() subset of _BUCKET_ORDER (the existing literal assertion in test_agent_analytics.py omits "Reminders", so it was never a completeness check); B6b mechanises the locked "headline avg is full-set, never the capped pool" discipline; B10 freezes iso_cutoff so the strict-'>' boundary is deterministic rather than a clock race. - test_1771c_schedules_analytics_properties.py: P-B1 conservation (sum of by_type == total_executions, over arbitrary unicode triggers), P-B2 rate/count bounds incl. the zero-terminal-day-is-None rule, P-B3 contiguous UTC-day timeline over arbitrary windows, P-B4 no-crash-total on the pure leaves. Hypothesis `event()` markers make non-vacuity provable via --hypothesis-show-statistics rather than assumed. Five first-run failures were all WRONG TESTS (my expectations/mechanisms), not product bugs — reflection notes recorded in the docstrings. Test-only. No src/ changes. Refs #1771 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(registry): register the four #1771 edge-case/property test files /update-tests tail step. Appended with ensure_ascii=True so the existing \uXXXX escaping of 24 unrelated entries is preserved — the diff is +54/-0. The companion .claude/agents/test-runner.md catalog sync is deliberately NOT done: that path is inside the pinned private .claude submodule, and this wave must not move the submodule gitlink. Refs #1771 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(db/schedules): make the B9 day-bucketing case date-relative (#1771) Self-review fix. B9 hard-coded a 2026-03-01/02 pair, which forced a 10-year window to keep the rows in range — so the gap-fill loop built ~3650 day-dicts per call, and the test would have aged out of any tighter window. Derive the straddling instant from today instead and use a 168h window. Same assertion, cannot age out; the file's runtime drops 8.3s -> 2.4s. Refs #1771 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(db/schedules): close the get_schedule_analytics gap and de-vacuum P-B2b (#1771) /review findings on the #1771 target-3 edge-case slice, fixed in place. Test-only: `git diff f7aff46..HEAD -- src/` stays empty (AC#4). R1 REQUIREMENTS MISSING — `get_schedule_analytics` was named a subject-under-test by the plan (§3) and by the edges file's own docstring, yet no test invoked it: under a new-tests-only coverage run its whole body (analytics.py:113-259) was unexecuted, and the reported 92% came from bundling nine pre-existing neighbour files. Added rows B13/B13b/B13c/B15. R2 Closed the residual gap the matrix itself flagged: the `tool_calls` JSON-SHAPE guards. The column is agent-written, so valid-JSON-not-a-list, list-of-scalars, dict-without-`name` and non-numeric `duration_ms` are all reachable input, and a regressed guard turns an analytics read into a 500. Neighbours cover only the `json.loads` raise. Now pinned on BOTH surfaces (`get_schedule_analytics` + `get_agent_schedules_summary`, 20 cases). R3 P-B2b was half-vacuous: it asserted `sampled is (eligible > cap)` with the cap at its production 5000 and at most 12 seeded rows, so the `True` arm was unreachable for every example. The cap is now drawn (1-6) and monkeypatched, with over/at/empty `@example` pins. Verified load-bearing — a probe forcing `sampled is False` fails on the explicit example. R7 Renamed the `st` loop variable that shadowed the conventional `hypothesis.strategies as st` alias used in the sibling properties file. Also corrected in `.plan/edge-cases-1771c-matrix.md` (untracked by convention): `analytics.py:234`/`241` were mis-described as tool_calls guards (234 is the empty-day guard, 241 the FAILED timeline arm, now covered); `542->550` is defensive-dead, not malformed-`started_at`-reachable (`'' > cutoff` is false in SQL, so an empty `started_at` never reaches the loop); both coverage numbers are now reported instead of only the bundled one; and the Hypothesis `event()` percentages are labelled as one observed run rather than a stable contract. Declined: ruff F811 on the `db_backend` fixture import (pre-existing repo-wide pattern in every db_harness consumer; no CI workflow runs ruff). Refs #1771 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(deps): harmonize the #1771 hypothesis block across the three slices The three concurrent #1771 slices each added `hypothesis==6.161.5` to tests/requirements-test.txt at a different position with a different comment, so they conflicted with each other rather than merging cleanly. Rebuild the file from dev's copy and insert one byte-identical canonical block at one fixed position (after pytest-cov). All three slices now carry the same bytes in the same place, so they merge in any order and the file keeps exactly ONE hypothesis line. No product code, no test logic change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(#1771): restore sys.modules the four 1771c files evict (#762 lint) The first CI run this PR was ever able to produce (it was CONFLICTING until now, so GitHub could not build refs/pull/1827/merge and reported zero checks) failed `lint (sys.modules pollution check)`: 12 bare `sys.modules.pop(...)` calls, 3 per new file. Fixed by satisfying the guard, NOT by widening its baseline (`lint_sys_modules_baseline.txt` is untouched) — this guard has already caught this same class in #783, #606 and #875, and retiring it on a test-rigour PR would be backwards. Both eviction sites are out of monkeypatch's reach, or actively wrong for it: * the `utils*` shadow clear is IMPORT-time, before any fixture exists, so monkeypatch structurally cannot reach it; * the `ops` fixture must evict `db.*` so `from db.schedules import ...` re-imports against the harness-bound engine. `monkeypatch.delitem` records NO undo for a key absent on entry (verified against _pytest.monkeypatch), so the freshly imported harness-bound module would stay resident for later files — strictly worse isolation than the explicit pop it would replace. So both take the documented escape hatch instead: a top-level `_STUBBED_MODULE_NAMES` list plus an autouse `_restore_sys_modules` fixture, matching the shape of tests/unit/test_telegram_webhook_backfill.py. It is a teardown-time guarantee only — the snapshot is taken before the test body, so no in-test behaviour and no assertion changes. Verified: lint reports "203 violation(s) in 60 file(s); baseline allows 240 — no new violations"; the four files still report exactly 129 passed, 1 xfailed (same strict xfail); `git diff origin/dev...HEAD -- src/` still empty. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

dolho and others added 2 commits May 11, 2026 15:45

dolho requested a review from vybe May 11, 2026 13:41

vybe merged commit 79a6481 into dev May 11, 2026
8 checks passed

vybe mentioned this pull request May 11, 2026

bug: cleanup watchdog re-verify defers indefinitely when agent unreachable, leaking slots under sustained overload #497

Closed

AndriiPasternak31 mentioned this pull request May 11, 2026

chore(tests): baseline pre-existing sys.modules pollution in test_cleanup_unreachable_orphan.py #796

Merged

3 tasks

This was referenced May 12, 2026

bug(tests): lint baseline rebase race — #791 CI passed against pre-#783 dev, post-merge dev broken #802

Closed

refactor(tests): extract shared isolated_module_loader for backend-module unit tests #803

Open

dolho mentioned this pull request May 13, 2026

ci(unit-test): run lint on push to dev/main to catch baseline rebase race #828

Merged

3 tasks

AndriiPasternak31 mentioned this pull request May 13, 2026

CI: sys.modules lint baseline out of sync with dev — blocks all downstream PRs #836

Closed

1 task

AndriiPasternak31 mentioned this pull request Jul 27, 2026

test(db/schedules): edge-case + property coverage for CAS status writers and analytics aggregation #1827

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(cleanup): force-fail orphan when agent unreachable during re-verify (#497) - #783

fix(cleanup): force-fail orphan when agent unreachable during re-verify (#497)#783
vybe merged 2 commits into
devfrom
investigate/497-cleanup-unreachable-deferral

dolho commented May 11, 2026

Uh oh!

dolho commented May 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

dolho commented May 11, 2026

Summary

Investigation

1. Traced the SUCCESS-write paths and race guards

2. Confirmed the slot "leak" framing was imprecise

3. Built an isolated repro

Documented residual risk

What stays unchanged

Test plan

Files

Uh oh!

dolho commented May 11, 2026

PR Validation Report (manual run of `/validate-pr` checklist)

Issues Found

Recommendation

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants