Leonid Pershin
1ec615c03e
- Marked critical bugs as resolved in the review documentation, including changes to the `arm_idle_killer` function to raise errors on credential creation failures and ensure proper file permissions for JSON credentials. - Introduced a new `_try_arm_idle_killer` function in `provision.py` to manage idle-killer state more effectively, ensuring it arms correctly during provisioning. - Updated the `swarm_busy` function in `remote/idle_killer.py` to allow idle state after a specified duration of Swarm unavailability, preventing unnecessary billing. - Enhanced performance tuning logic in `tune_swarm_perf.py` to ensure proper handling of pip installation success before applying extra arguments. - Added tests to validate the new idle-killer behavior and swarm management logic, ensuring robustness in handling idle states and error conditions.
91 lines
3.9 KiB
Markdown
91 lines
3.9 KiB
Markdown
# Project Review — 2026-08-21 (3)
|
||
|
||
Critical-only pass (мелочи / style / UX polish ignored). Focus: billing safety, silent GPU left on, security of secrets, broken stop/auto-stop paths.
|
||
|
||
## Prior Reviews Summary
|
||
|
||
> Based on `2026-08-21-review-2.md` and `2026-08-21-review-1.md`.
|
||
|
||
### Still Open (carried forward)
|
||
|
||
None. All tasks in review-1 and review-2 are `[x]`.
|
||
|
||
### Resolved Since Last Review
|
||
|
||
- [x] Prior review-2 closed the fail-closed idle-killer access_rules + revoke on stop, Ollama pull exact-tag, provision_llm not swallowed, etc. Those remain in place; this review finds **new** regressions/gaps around arm false-positive and unreachable Swarm.
|
||
|
||
---
|
||
|
||
## Phase 1: Code Quality
|
||
|
||
### SOLID
|
||
|
||
No critical issues (god-flow in `session`/`provision` is known debt, not a break).
|
||
|
||
### Performance
|
||
|
||
No critical issues.
|
||
|
||
### Correctness & Bugs
|
||
|
||
1. **`arm_idle_killer` soft-fails on missing app cred, but provision still marks `armed`.**
|
||
`create_application_credential` failure is caught, logged (`idle-killer слеп`), and `return`s without raising. Caller always sets `notes["idle_killer"] = "armed"`. Access card only warns on `"failed"`. → User thinks auto-stop works; GPU bills forever.
|
||
|
||
2. **Remote idle-killer treats any Swarm HTTP error as busy forever.**
|
||
`swarm_busy`: unreachable → `(True, "swarm unreachable…")`, which resets idle timer every tick. No grace/timeout to delete when Swarm is down for hours. → Crashed Swarm / bad Comfy ExtraArgs → never delete.
|
||
|
||
3. **`provision_llm` / seed failures can leave ACTIVE compute before killer arm.**
|
||
Killer is armed last in `provision_vm`. Earlier `CloudError` aborts `up` with server already created and no timer. Known risk amplified by LLM/extensions paths.
|
||
|
||
4. **Perf tune sets `pip_ok=True` even when `pip install triton/sageattention` fails**, then may still patch `--use-sage-attention` and restart Swarm. Marker prevents retry. → Can leave Swarm broken → compounds (2).
|
||
|
||
### Code Quality
|
||
|
||
No critical issues.
|
||
|
||
---
|
||
|
||
## Phase 2: Logical Consistency
|
||
|
||
### Domain & Application Layer
|
||
|
||
No critical issues.
|
||
|
||
### Data Flow
|
||
|
||
Requires-filter for extensions / Ollama install paths / balance notify (notify-only) — no critical billing harm found.
|
||
|
||
### State Management
|
||
|
||
`notes.idle_killer = "armed"` does not match actual arm success (see bug 1).
|
||
|
||
### Consistency
|
||
|
||
No critical issues.
|
||
|
||
---
|
||
|
||
## Phase 3: UI/UX
|
||
|
||
### Usability
|
||
|
||
Misleading “armed” status when killer is blind — treated as Bug/Logic above, not UX polish.
|
||
|
||
### Visual / Interaction / Accessibility
|
||
|
||
N/A for CLI critical pass (or no critical issues).
|
||
|
||
---
|
||
|
||
## Tasks
|
||
|
||
Critical only:
|
||
|
||
- [x] 1. [Bug] `arm_idle_killer`: on cred create failure **raise** or return False; never set `notes.idle_killer=armed`; surface same ⚠ as `"failed"` — `src/gpu_rent/idle_killer.py` line 151, `src/gpu_rent/provision.py` line 516
|
||
- [x] 2. [Bug] Idle-killer: if Swarm unreachable longer than N minutes (e.g. 2× idle or fixed 60m), treat as idle/allow delete (llm-only already bypasses) — `src/gpu_rent/remote/idle_killer.py` line 75
|
||
- [x] 3. [Logic] On mid-`up` failure after server create, arm killer anyway or fail loudly and refuse to leave session without killer / document mandatory `stop` — `src/gpu_rent/provision.py` / `session.py`
|
||
- [x] 4. [Bug] Perf tune: set `pip_ok` only if pip succeeded; do **not** write `--use-sage-attention` ExtraArgs unless install OK (or allow retry when `pip_ok` false) — `src/gpu_rent/remote/tune_swarm_perf.py` line 109
|
||
- [x] 5. [Security] Write idle-killer creds JSON with `mode=0o600` before `mv` (same as GIT_TOKEN) — `src/gpu_rent/idle_killer.py` line 156
|
||
|
||
**Verified OK (critical):** `cmd_stop` delete path, tunnel Ctrl+C detach, local-watchdog stop on stale lease, `requires: ollama` filter, balance notify (toast only), Ollama unit bind to data dir, 127 tests green at review time.
|