Enforce preflight gate, add runtime bootstrap, DB integrity check, health daemon, and test matrix#86
Open
NguyenCuong1989 wants to merge 1 commit intomainfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Hướng Dẫn Cho Người Review (Reviewer's Guide)Triển khai một framework ổn định runtime có tính quyết định bằng cách giới thiệu một “preflight health gate”, một script bootstrap chuẩn bị các thư mục runtime và DB SQLite (WAL), một trình chạy DNR hằng ngày, một health daemon định kỳ với các marker “unhealthy”, xoay vòng log (log rotation), cùng ma trận test runtime tự động và unit test để thực thi nghiêm ngặt semantics mã thoát 0/1/2. ER diagram cho các bảng SQLite runtime được DNR và bootstrap sử dụngerDiagram
runtime_bootstrap {
INTEGER id PK
TEXT boot_at
}
dnr_runs {
INTEGER id PK
TEXT run_at
}
%% No explicit foreign keys; tables are independent
runtime_bootstrap ||--o{ dnr_runs : records_separate_events
Class diagram cho locking và DB helpers trong daily_dnr_run.pyclassDiagram
class CycleLock {
-Path path
-int fd
+CycleLock(path)
+__enter__() CycleLock
+__exit__(exc_type, exc, tb) void
}
class DailyDnrModule {
+parse_args() argparse_Namespace
+configure_logging(log_dir) void
+marker_for_today(marker_dir) Path
+mark_cycle_complete(marker) void
+wal_marker_for_db(db_path, state_dir) Path
+ensure_wal_initialized(db_path, state_dir) void
+connect_db(db_path, state_dir) sqlite3_Connection
+run_cycle(db_path, marker_dir, lock_file, state_dir) int
+run_preflight_gate() int
+main() int
}
DailyDnrModule "1" o-- "1" CycleLock : uses_for_exclusive_cycle
DailyDnrModule "1" o-- "1" sqlite3_Connection : manages_db_connection
Thay Đổi Ở Cấp Độ File
Tips và lệnh hữu íchTương tác với Sourcery
Tùy chỉnh Trải nghiệm của BạnTruy cập dashboard để:
Nhận Hỗ Trợ
Original review guide in EnglishReviewer's GuideImplements a deterministic runtime stability framework by introducing a preflight health gate, a bootstrap script that prepares runtime directories and SQLite (WAL) DB, a daily DNR runner, a periodic health daemon with unhealthy markers, log rotation, and an automated runtime test matrix plus unit tests to enforce strict 0/1/2 exit semantics. ER diagram for runtime SQLite tables used by DNR and bootstraperDiagram
runtime_bootstrap {
INTEGER id PK
TEXT boot_at
}
dnr_runs {
INTEGER id PK
TEXT run_at
}
%% No explicit foreign keys; tables are independent
runtime_bootstrap ||--o{ dnr_runs : records_separate_events
Class diagram for daily_dnr_run.py locking and DB helpersclassDiagram
class CycleLock {
-Path path
-int fd
+CycleLock(path)
+__enter__() CycleLock
+__exit__(exc_type, exc, tb) void
}
class DailyDnrModule {
+parse_args() argparse_Namespace
+configure_logging(log_dir) void
+marker_for_today(marker_dir) Path
+mark_cycle_complete(marker) void
+wal_marker_for_db(db_path, state_dir) Path
+ensure_wal_initialized(db_path, state_dir) void
+connect_db(db_path, state_dir) sqlite3_Connection
+run_cycle(db_path, marker_dir, lock_file, state_dir) int
+run_preflight_gate() int
+main() int
}
DailyDnrModule "1" o-- "1" CycleLock : uses_for_exclusive_cycle
DailyDnrModule "1" o-- "1" sqlite3_Connection : manages_db_connection
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||||||||||
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - tôi đã tìm thấy 6 vấn đề và để lại một số phản hồi tổng quan:
- Trong
preflight.shgiá trị mặc định củaDISK_PATHđang được hardcode thành/Users/andy, khiến bước kiểm tra dung lượng đĩa không dùng lại được trên các máy khác; hãy cân nhắc đặt mặc định là$HOMEhoặc một đường dẫn tổng quát hơn để lần kiểm tra này hoạt động đúng trên nhiều môi trường người dùng khác nhau. - Trong
launchd_safe_wrapper.shgiá trị fallback củaSCRIPT_PATHđang mặc định là$HOME/workbench/daily_dnr_run.py, có vẻ phụ thuộc vào repo/người dùng cụ thể; có thể sẽ ổn định hơn nếu mặc định trỏ tới file được tracktools/runtime/daily_dnr_run.pyhoặc yêu cầu truyền đường dẫn rõ ràng.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Trong `preflight.sh` giá trị mặc định của `DISK_PATH` đang được hardcode thành `/Users/andy`, khiến bước kiểm tra dung lượng đĩa không dùng lại được trên các máy khác; hãy cân nhắc đặt mặc định là `$HOME` hoặc một đường dẫn tổng quát hơn để lần kiểm tra này hoạt động đúng trên nhiều môi trường người dùng khác nhau.
- Trong `launchd_safe_wrapper.sh` giá trị fallback của `SCRIPT_PATH` đang mặc định là `$HOME/workbench/daily_dnr_run.py`, có vẻ phụ thuộc vào repo/người dùng cụ thể; có thể sẽ ổn định hơn nếu mặc định trỏ tới file được track `tools/runtime/daily_dnr_run.py` hoặc yêu cầu truyền đường dẫn rõ ràng.
## Individual Comments
### Comment 1
<location> `tools/runtime/daily_dnr_run.py:131-148` </location>
<code_context>
+
+
+
+def run_preflight_gate() -> int:
+ if os.environ.get("SKIP_PREFLIGHT", "0") == "1":
+ return 0
+
+ try:
+ result = subprocess.run(
+ ["./tools/runtime/preflight.sh", "--summary"],
+ capture_output=True,
+ text=True,
+ check=False,
+ )
+ if result.stdout:
+ print(result.stdout.strip())
+ if result.stderr:
+ print(result.stderr.strip())
+ return int(result.returncode)
+ except Exception:
+ return 1
+
</code_context>
<issue_to_address>
**suggestion:** Hãy cân nhắc log khi chính việc gọi preflight bị lỗi thay vì âm thầm trả về 1.
Hiện tại mọi ngoại lệ từ `subprocess.run` (thiếu script, quyền truy cập, v.v.) đều được ánh xạ thành mã thoát `1` mà không có ngữ cảnh, khiến lỗi hạ tầng không phân biệt được với một WARN thực sự. Vui lòng log ngoại lệ hoặc một thông báo lỗi ngắn trước khi trả về để bên gọi có thể phân biệt lỗi preflight với lỗi runtime/môi trường khi diễn giải mã thoát.
```suggestion
def run_preflight_gate() -> int:
if os.environ.get("SKIP_PREFLIGHT", "0") == "1":
return 0
try:
result = subprocess.run(
["./tools/runtime/preflight.sh", "--summary"],
capture_output=True,
text=True,
check=False,
)
if result.stdout:
print(result.stdout.strip())
if result.stderr:
print(result.stderr.strip())
return int(result.returncode)
except Exception:
logging.exception("Preflight gate invocation failed (unable to run preflight.sh)")
return 1
```
</issue_to_address>
### Comment 2
<location> `tools/runtime/runtime_health_daemon.py:36-37` </location>
<code_context>
+ handle.write(json.dumps(payload, ensure_ascii=False) + "\n")
+
+
+def run_preflight() -> int:
+ result = subprocess.run(["./tools/runtime/preflight.sh", "--summary"], capture_output=True, text=True)
+ if result.stdout:
+ print(result.stdout.strip())
</code_context>
<issue_to_address>
**issue (bug_risk):** Việc dùng đường dẫn tương đối cho `preflight.sh` khiến daemon phụ thuộc vào thư mục làm việc hiện tại.
`subprocess.run([
</issue_to_address>
### Comment 3
<location> `tools/runtime/preflight.sh:5` </location>
<code_context>
+set -euo pipefail
+
+MIN_FREE_GB="${MIN_FREE_GB:-15}"
+DISK_PATH="${DISK_PATH:-/Users/andy}"
+DB_PATH="${DB_PATH:-$HOME/.hyperai/db/memory.sqlite}"
+LOG_DIR="${LOG_DIR:-$HOME/.hyperai/logs}"
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Giá trị mặc định của `DISK_PATH` gắn với người dùng cụ thể và có thể không chạy được trên các máy khác.
Hãy cân nhắc đặt mặc định thành một đường dẫn tổng quát hơn (ví dụ `${DISK_PATH:-$HOME}` hoặc tương tự) để preflight có thể dùng lại được, đồng thời vẫn cho phép override qua biến môi trường.
```suggestion
DISK_PATH="${DISK_PATH:-$HOME}"
```
</issue_to_address>
### Comment 4
<location> `tools/runtime/launchd_safe_wrapper.sh:4` </location>
<code_context>
+#!/usr/bin/env bash
+set -euo pipefail
+
+SCRIPT_PATH="${1:-${ORIG:-$HOME/workbench/daily_dnr_run.py}}"
+if [[ $# -gt 0 ]]; then
+ shift
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Giá trị mặc định của `SCRIPT_PATH` trỏ tới một đường dẫn gần như chỉ tồn tại cục bộ, có thể không ổn định.
Vì `$HOME/workbench/daily_dnr_run.py` phụ thuộc vào môi trường, nên khi `ORIG` không được set và không truyền tham số vị trí nào, script này rất có thể sẽ lỗi trong hầu hết môi trường. Hãy cân nhắc đặt mặc định thành một đường dẫn bên trong repo (ví dụ `./tools/runtime/daily_dnr_run.py` được resolve tương đối so với wrapper) để script có một giá trị mặc định có thể dùng lại và có thể dự đoán được.
```suggestion
SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" >/dev/null 2>&1 && pwd)"
SCRIPT_PATH="${1:-${ORIG:-"$SCRIPT_DIR/daily_dnr_run.py"}}"
```
</issue_to_address>
### Comment 5
<location> `tests/runtime/test_runtime_tools.py:22-31` </location>
<code_context>
+ def test_daily_dnr_idempotent_same_day(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Thêm các bài test DNR cho trường hợp thiếu DB, tranh chấp lock, và các nhánh lỗi SQLite
Để kiểm thử đầy đủ hợp đồng mã thoát của `daily_dnr_run.py`, vui lòng thêm các bài test:
- Bao phủ trường hợp thiếu đường dẫn DB và assert WARN (thoát 1) cùng log cảnh báo.
- Mô phỏng tranh chấp lock (giữ trước file lock) và assert nhánh `RuntimeError` cũng cho kết quả WARN (thoát 1).
- Gây ra `sqlite3.Error` (ví dụ file DB hỏng hoặc lỗi quyền) và assert FAIL (thoát 2).
Điều này đảm bảo tất cả các nhánh được mô tả trong `main()` đều được kiểm chứng bằng test.
Gợi ý triển khai:
```python
DAEMON = ROOT / "tools/runtime/runtime_health_daemon.py"
DAILY_DNR = ROOT / "tools/runtime/daily_dnr_run.py"
```
```python
def test_preflight_fail_on_invalid_disk_path(self):
env = os.environ.copy()
env["DISK_PATH"] = "/path/does/not/exist"
result = subprocess.run([str(PREFLIGHT), "--summary"], cwd=ROOT, env=env, capture_output=True, text=True)
self.assertEqual(result.returncode, 2)
def test_daily_dnr_idempotent_same_day(self):
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
db = td_path / "memory.sqlite"
log_dir = td_path / "logs"
marker_dir = td_path / "state/dnr"
lock_file = td_path / "state/dnr.lock"
db.parent.mkdir(parents=True, exist_ok=True)
sqlite3.connect(db).close()
env = os.environ.copy()
env["DAILY_DNR_DB_PATH"] = str(db)
env["DAILY_DNR_LOG_DIR"] = str(log_dir)
env["DAILY_DNR_MARKER_DIR"] = str(marker_dir)
env["DAILY_DNR_LOCK_FILE"] = str(lock_file)
# First run should succeed and create the marker/lock state
result1 = subprocess.run(
[sys.executable, str(DAILY_DNR)],
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
self.assertEqual(result1.returncode, 0)
# Second run on the same day should be idempotent and still succeed
result2 = subprocess.run(
[sys.executable, str(DAILY_DNR)],
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
self.assertEqual(result2.returncode, 0)
def test_daily_dnr_warns_on_missing_db(self):
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
db = td_path / "missing.sqlite"
log_dir = td_path / "logs"
marker_dir = td_path / "state/dnr"
lock_file = td_path / "state/dnr.lock"
env = os.environ.copy()
env["DAILY_DNR_DB_PATH"] = str(db)
env["DAILY_DNR_LOG_DIR"] = str(log_dir)
env["DAILY_DNR_MARKER_DIR"] = str(marker_dir)
env["DAILY_DNR_LOCK_FILE"] = str(lock_file)
result = subprocess.run(
[sys.executable, str(DAILY_DNR)],
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
# WARN: Missing DB should exit 1 and emit a warning on stderr
self.assertEqual(result.returncode, 1)
self.assertTrue(result.stderr, "Expected warning output when DB path is missing")
def test_daily_dnr_warns_on_lock_contention(self):
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
db = td_path / "memory.sqlite"
log_dir = td_path / "logs"
marker_dir = td_path / "state/dnr"
lock_file = td_path / "state/dnr.lock"
db.parent.mkdir(parents=True, exist_ok=True)
sqlite3.connect(db).close()
env = os.environ.copy()
env["DAILY_DNR_DB_PATH"] = str(db)
env["DAILY_DNR_LOG_DIR"] = str(log_dir)
env["DAILY_DNR_MARKER_DIR"] = str(marker_dir)
env["DAILY_DNR_LOCK_FILE"] = str(lock_file)
# Pre-hold the lock file to simulate another process owning it
lock_file.parent.mkdir(parents=True, exist_ok=True)
with open(lock_file, "w") as fh:
try:
import fcntl
fcntl.flock(fh, fcntl.LOCK_EX | fcntl.LOCK_NB)
except (ImportError, OSError):
# If flock is not available or locking fails for platform reasons,
# still keep the file present to exercise the lock contention path
pass
result = subprocess.run(
[sys.executable, str(DAILY_DNR)],
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
# WARN: lock contention is treated as RuntimeError -> exit 1
self.assertEqual(result.returncode, 1)
self.assertTrue(result.stderr, "Expected warning output when lock contention occurs")
def test_daily_dnr_fails_on_sqlite_error(self):
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
# Point DB at a directory to provoke a sqlite3.Error when connecting
db = td_path / "not_a_file.sqlite"
db.mkdir(parents=True, exist_ok=True)
log_dir = td_path / "logs"
marker_dir = td_path / "state/dnr"
lock_file = td_path / "state/dnr.lock"
env = os.environ.copy()
env["DAILY_DNR_DB_PATH"] = str(db)
env["DAILY_DNR_LOG_DIR"] = str(log_dir)
env["DAILY_DNR_MARKER_DIR"] = str(marker_dir)
env["DAILY_DNR_LOCK_FILE"] = str(lock_file)
result = subprocess.run(
[sys.executable, str(DAILY_DNR)],
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
# FAIL: sqlite3.Error paths should exit 2 and log an error
self.assertEqual(result.returncode, 2)
self.assertTrue(result.stderr, "Expected error output when sqlite3.Error is raised")
```
1. Đảm bảo `DAILY_DNR` là hằng đường dẫn đúng cho `daily_dnr_run.py`; nếu file này đã có một hằng khác (ví dụ `DAILY_DNR_RUN`), hãy tái sử dụng thay vì tạo hằng mới.
2. Đồng bộ tên các biến môi trường (`DAILY_DNR_DB_PATH`, `DAILY_DNR_LOG_DIR`, `DAILY_DNR_MARKER_DIR`, `DAILY_DNR_LOCK_FILE`) với những gì `daily_dnr_run.main()` thực sự đọc; cập nhật các key trong cả bốn bài test cho phù hợp.
3. Nếu `daily_dnr_run.py` được gọi trực tiếp như một script thực thi (shebang + `chmod +x`), và các test khác gọi nó mà không qua `sys.executable`, hãy cập nhật các lệnh `subprocess.run` cho phù hợp với convention hiện tại.
4. Nếu thông báo cảnh báo/lỗi đã biết và ổn định, hãy tăng độ chặt cho các assert trên `stderr` bằng cách kiểm tra các substring cụ thể (ví dụ "WARN" hoặc thông điệp log cụ thể) thay vì chỉ kiểm tra là có output.
</issue_to_address>
### Comment 6
<location> `tests/runtime/test_runtime_tools.py:53-62` </location>
<code_context>
+ def test_health_daemon_writes_trend(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Mở rộng các test cho health daemon để kiểm tra nội dung sự kiện, ngưỡng và hành vi phục hồi
Bài test hiện tại chỉ assert rằng một chu kỳ FAIL đơn lẻ tạo ra `runtime_health_trend.json` và marker unhealthy. Để kiểm thử hành vi của daemon tốt hơn, hãy cân nhắc:
- Assert rằng entry JSON trong trend chứa các trường mong đợi (`exit_code`, `state`, `consecutive_failures`, và tùy chọn `event == 'state_changed'`).
- Thêm một bài test đa chu kỳ với `max-cycles > 1` để xác minh `threshold` được tôn trọng (ví dụ threshold=2 chỉ đánh dấu unhealthy sau 2 FAIL liên tiếp).
- Thêm một bài test phục hồi, trong đó một chu kỳ FAIL được theo sau bởi một chu kỳ không FAIL, assert rằng `consecutive_failures` được reset và bất kỳ `system_unhealthy.marker` hiện có nào được xóa.
Điều này sẽ giúp kiểm chứng đầy đủ ngữ nghĩa watchdog chạy lâu dài.
Gợi ý triển khai:
```python
import os
import json
import subprocess
import tempfile
from pathlib import Path
```
```python
def test_health_daemon_writes_trend(self):
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
state_dir = td_path / "state"
state_dir.mkdir(parents=True, exist_ok=True)
log_dir = td_path / "logs"
log_dir.mkdir(parents=True, exist_ok=True)
log_file = log_dir / "daemon.log"
# Force the daemon into a failing state so we exercise the
# unhealthy path and trend writing.
env = os.environ.copy()
env["DISK_PATH"] = "/path/does/not/exist"
cmd = [
"python3",
str(DAEMON),
"--state-dir",
str(state_dir),
"--log-file",
str(log_file),
"--max-cycles",
"1",
"--threshold",
"1",
]
result = subprocess.run(
cmd,
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
self.assertEqual(result.returncode, 0, msg=result.stderr)
trend_path = state_dir / "runtime_health_trend.json"
self.assertTrue(trend_path.exists())
# Validate that at least one event was written and that it contains
# the core fields we expect for watchdog semantics.
with trend_path.open() as f:
events = [json.loads(line) for line in f if line.strip()]
self.assertGreaterEqual(len(events), 1)
last = events[-1]
self.assertIn("exit_code", last)
self.assertIn("state", last)
self.assertIn("consecutive_failures", last)
if "event" in last:
self.assertEqual(last["event"], "state_changed")
marker_path = state_dir / "system_unhealthy.marker"
self.assertTrue(marker_path.exists())
def test_health_daemon_honors_failure_threshold(self):
"""Verify that threshold > 1 only marks unhealthy after enough consecutive failures."""
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
state_dir = td_path / "state"
state_dir.mkdir(parents=True, exist_ok=True)
log_dir = td_path / "logs"
log_dir.mkdir(parents=True, exist_ok=True)
log_file = log_dir / "daemon.log"
env = os.environ.copy()
env["DISK_PATH"] = "/path/does/not/exist"
cmd = [
"python3",
str(DAEMON),
"--state-dir",
str(state_dir),
"--log-file",
str(log_file),
"--max-cycles",
"3",
"--threshold",
"2",
]
result = subprocess.run(
cmd,
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
self.assertEqual(result.returncode, 0, msg=result.stderr)
trend_path = state_dir / "runtime_health_trend.json"
self.assertTrue(trend_path.exists())
with trend_path.open() as f:
events = [json.loads(line) for line in f if line.strip()]
# We expect multiple cycles to have been recorded.
self.assertGreaterEqual(len(events), 2)
# Find the last event for the failing probe and assert that the
# consecutive failure count reached at least the threshold.
last = events[-1]
self.assertIn("consecutive_failures", last)
self.assertGreaterEqual(last["consecutive_failures"], 2)
# Once threshold is reached, the system must be marked unhealthy.
marker_path = state_dir / "system_unhealthy.marker"
self.assertTrue(marker_path.exists())
def test_health_daemon_recovers_and_resets_failures(self):
"""Verify that a recovery cycle resets consecutive_failures and clears the unhealthy marker."""
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
state_dir = td_path / "state"
state_dir.mkdir(parents=True, exist_ok=True)
log_dir = td_path / "logs"
log_dir.mkdir(parents=True, exist_ok=True)
log_file = log_dir / "daemon.log"
# First run: force failures so we cross the threshold.
env_fail = os.environ.copy()
env_fail["DISK_PATH"] = "/path/does/not/exist"
cmd = [
"python3",
str(DAEMON),
"--state-dir",
str(state_dir),
"--log-file",
str(log_file),
"--max-cycles",
"3",
"--threshold",
"2",
]
result_fail = subprocess.run(
cmd,
cwd=ROOT,
env=env_fail,
capture_output=True,
text=True,
)
self.assertEqual(result_fail.returncode, 0, msg=result_fail.stderr)
marker_path = state_dir / "system_unhealthy.marker"
self.assertTrue(marker_path.exists())
# Second run: allow the daemon to succeed so it can recover.
env_ok = os.environ.copy()
# Do not override DISK_PATH (or set it to a valid path) so the probe passes.
result_ok = subprocess.run(
cmd,
cwd=ROOT,
env=env_ok,
capture_output=True,
text=True,
)
self.assertEqual(result_ok.returncode, 0, msg=result_ok.stderr)
trend_path = state_dir / "runtime_health_trend.json"
self.assertTrue(trend_path.exists())
with trend_path.open() as f:
events = [json.loads(line) for line in f if line.strip()]
self.assertGreaterEqual(len(events), 1)
last = events[-1]
# Recovery should reset the failure counter.
self.assertIn("consecutive_failures", last)
self.assertEqual(last["consecutive_failures"], 0)
# Recovery should also clear any unhealthy marker.
self.assertFalse(marker_path.exists())
```
Các lệnh và tên trường JSON trong các bài test này giả định daemon hỗ trợ các tùy chọn CLI và schema sự kiện sau:
- Cờ CLI: `--state-dir`, `--log-file`, `--max-cycles`, `--threshold`.
- Các trường sự kiện JSON: ít nhất `exit_code`, `state`, `consecutive_failures`, và tùy chọn `event == "state_changed"` khi chuyển trạng thái.
- Một file marker tên `system_unhealthy.marker` được tạo/xóa dưới `state_dir`.
Nếu CLI hoặc schema thực tế khác, bạn nên:
1. Điều chỉnh mảng `cmd` (các cờ/giá trị) cho khớp với các tham số thật.
2. Cập nhật các key JSON được assert (`exit_code`, `state`, `consecutive_failures`, `event`) thành các tên trường thật sự mà daemon sử dụng.
3. Cập nhật tên file marker (`system_unhealthy.marker`) và kỳ vọng về vòng đời của nó (khi nào được tạo/xóa) để khớp với triển khai thực tế.
4. Nếu hành vi phục hồi dùng cơ chế khác với việc reset `consecutive_failures` về `0`, hãy điều chỉnh các assert cuối cùng cho phù hợp.
</issue_to_address>Sourcery miễn phí cho open source - nếu bạn thấy hữu ích, hãy cân nhắc chia sẻ ✨
Original comment in English
Hey - I've found 6 issues, and left some high level feedback:
- In
preflight.shthe defaultDISK_PATHis hardcoded to/Users/andy, which makes the disk gate non-portable; consider defaulting to$HOMEor a more generic path so the check behaves correctly across different user environments. - In
launchd_safe_wrapper.shthe fallbackSCRIPT_PATHdefaults to$HOME/workbench/daily_dnr_run.py, which looks repo/user-specific; it may be more robust to default to the trackedtools/runtime/daily_dnr_run.pyor require the path be passed explicitly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `preflight.sh` the default `DISK_PATH` is hardcoded to `/Users/andy`, which makes the disk gate non-portable; consider defaulting to `$HOME` or a more generic path so the check behaves correctly across different user environments.
- In `launchd_safe_wrapper.sh` the fallback `SCRIPT_PATH` defaults to `$HOME/workbench/daily_dnr_run.py`, which looks repo/user-specific; it may be more robust to default to the tracked `tools/runtime/daily_dnr_run.py` or require the path be passed explicitly.
## Individual Comments
### Comment 1
<location> `tools/runtime/daily_dnr_run.py:131-148` </location>
<code_context>
+
+
+
+def run_preflight_gate() -> int:
+ if os.environ.get("SKIP_PREFLIGHT", "0") == "1":
+ return 0
+
+ try:
+ result = subprocess.run(
+ ["./tools/runtime/preflight.sh", "--summary"],
+ capture_output=True,
+ text=True,
+ check=False,
+ )
+ if result.stdout:
+ print(result.stdout.strip())
+ if result.stderr:
+ print(result.stderr.strip())
+ return int(result.returncode)
+ except Exception:
+ return 1
+
</code_context>
<issue_to_address>
**suggestion:** Consider logging when preflight invocation itself fails instead of silently returning 1.
Right now any exception from `subprocess.run` (missing script, permissions, etc.) is mapped to exit code `1` with no context, making infra failures indistinguishable from a real WARN. Please log the exception or a short error message before returning so callers can differentiate preflight failures from runtime/environment issues when interpreting the exit code.
```suggestion
def run_preflight_gate() -> int:
if os.environ.get("SKIP_PREFLIGHT", "0") == "1":
return 0
try:
result = subprocess.run(
["./tools/runtime/preflight.sh", "--summary"],
capture_output=True,
text=True,
check=False,
)
if result.stdout:
print(result.stdout.strip())
if result.stderr:
print(result.stderr.strip())
return int(result.returncode)
except Exception:
logging.exception("Preflight gate invocation failed (unable to run preflight.sh)")
return 1
```
</issue_to_address>
### Comment 2
<location> `tools/runtime/runtime_health_daemon.py:36-37` </location>
<code_context>
+ handle.write(json.dumps(payload, ensure_ascii=False) + "\n")
+
+
+def run_preflight() -> int:
+ result = subprocess.run(["./tools/runtime/preflight.sh", "--summary"], capture_output=True, text=True)
+ if result.stdout:
+ print(result.stdout.strip())
</code_context>
<issue_to_address>
**issue (bug_risk):** Using a relative path for `preflight.sh` makes the daemon sensitive to the working directory.
`subprocess.run([
</issue_to_address>
### Comment 3
<location> `tools/runtime/preflight.sh:5` </location>
<code_context>
+set -euo pipefail
+
+MIN_FREE_GB="${MIN_FREE_GB:-15}"
+DISK_PATH="${DISK_PATH:-/Users/andy}"
+DB_PATH="${DB_PATH:-$HOME/.hyperai/db/memory.sqlite}"
+LOG_DIR="${LOG_DIR:-$HOME/.hyperai/logs}"
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The default `DISK_PATH` is user-specific and may not work on other machines.
Consider defaulting to a more generic path (e.g., `${DISK_PATH:-$HOME}` or similar) so the preflight remains portable while still allowing environment overrides.
```suggestion
DISK_PATH="${DISK_PATH:-$HOME}"
```
</issue_to_address>
### Comment 4
<location> `tools/runtime/launchd_safe_wrapper.sh:4` </location>
<code_context>
+#!/usr/bin/env bash
+set -euo pipefail
+
+SCRIPT_PATH="${1:-${ORIG:-$HOME/workbench/daily_dnr_run.py}}"
+if [[ $# -gt 0 ]]; then
+ shift
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Default `SCRIPT_PATH` points to a likely local-only path, which may be brittle.
Because `$HOME/workbench/daily_dnr_run.py` is environment-specific, when `ORIG` is unset and no positional argument is provided this will likely break in most environments. Consider defaulting to a path within the repo (e.g., `./tools/runtime/daily_dnr_run.py` resolved relative to the wrapper) so the script has a portable, predictable default.
```suggestion
SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" >/dev/null 2>&1 && pwd)"
SCRIPT_PATH="${1:-${ORIG:-"$SCRIPT_DIR/daily_dnr_run.py"}}"
```
</issue_to_address>
### Comment 5
<location> `tests/runtime/test_runtime_tools.py:22-31` </location>
<code_context>
+ def test_daily_dnr_idempotent_same_day(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Add DNR tests for missing DB, lock contention, and SQLite error paths
To fully exercise `daily_dnr_run.py`’s exit contract, please add tests that:
- Cover a missing DB path and assert WARN (exit 1) plus the warning log.
- Simulate lock contention (pre-hold the lock file) and assert the `RuntimeError` path also yields WARN (exit 1).
- Trigger a `sqlite3.Error` (e.g., bad DB file or permissions) and assert FAIL (exit 2).
This will ensure all documented branches in `main()` are verified by tests.
Suggested implementation:
```python
DAEMON = ROOT / "tools/runtime/runtime_health_daemon.py"
DAILY_DNR = ROOT / "tools/runtime/daily_dnr_run.py"
```
```python
def test_preflight_fail_on_invalid_disk_path(self):
env = os.environ.copy()
env["DISK_PATH"] = "/path/does/not/exist"
result = subprocess.run([str(PREFLIGHT), "--summary"], cwd=ROOT, env=env, capture_output=True, text=True)
self.assertEqual(result.returncode, 2)
def test_daily_dnr_idempotent_same_day(self):
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
db = td_path / "memory.sqlite"
log_dir = td_path / "logs"
marker_dir = td_path / "state/dnr"
lock_file = td_path / "state/dnr.lock"
db.parent.mkdir(parents=True, exist_ok=True)
sqlite3.connect(db).close()
env = os.environ.copy()
env["DAILY_DNR_DB_PATH"] = str(db)
env["DAILY_DNR_LOG_DIR"] = str(log_dir)
env["DAILY_DNR_MARKER_DIR"] = str(marker_dir)
env["DAILY_DNR_LOCK_FILE"] = str(lock_file)
# First run should succeed and create the marker/lock state
result1 = subprocess.run(
[sys.executable, str(DAILY_DNR)],
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
self.assertEqual(result1.returncode, 0)
# Second run on the same day should be idempotent and still succeed
result2 = subprocess.run(
[sys.executable, str(DAILY_DNR)],
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
self.assertEqual(result2.returncode, 0)
def test_daily_dnr_warns_on_missing_db(self):
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
db = td_path / "missing.sqlite"
log_dir = td_path / "logs"
marker_dir = td_path / "state/dnr"
lock_file = td_path / "state/dnr.lock"
env = os.environ.copy()
env["DAILY_DNR_DB_PATH"] = str(db)
env["DAILY_DNR_LOG_DIR"] = str(log_dir)
env["DAILY_DNR_MARKER_DIR"] = str(marker_dir)
env["DAILY_DNR_LOCK_FILE"] = str(lock_file)
result = subprocess.run(
[sys.executable, str(DAILY_DNR)],
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
# WARN: Missing DB should exit 1 and emit a warning on stderr
self.assertEqual(result.returncode, 1)
self.assertTrue(result.stderr, "Expected warning output when DB path is missing")
def test_daily_dnr_warns_on_lock_contention(self):
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
db = td_path / "memory.sqlite"
log_dir = td_path / "logs"
marker_dir = td_path / "state/dnr"
lock_file = td_path / "state/dnr.lock"
db.parent.mkdir(parents=True, exist_ok=True)
sqlite3.connect(db).close()
env = os.environ.copy()
env["DAILY_DNR_DB_PATH"] = str(db)
env["DAILY_DNR_LOG_DIR"] = str(log_dir)
env["DAILY_DNR_MARKER_DIR"] = str(marker_dir)
env["DAILY_DNR_LOCK_FILE"] = str(lock_file)
# Pre-hold the lock file to simulate another process owning it
lock_file.parent.mkdir(parents=True, exist_ok=True)
with open(lock_file, "w") as fh:
try:
import fcntl
fcntl.flock(fh, fcntl.LOCK_EX | fcntl.LOCK_NB)
except (ImportError, OSError):
# If flock is not available or locking fails for platform reasons,
# still keep the file present to exercise the lock contention path
pass
result = subprocess.run(
[sys.executable, str(DAILY_DNR)],
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
# WARN: lock contention is treated as RuntimeError -> exit 1
self.assertEqual(result.returncode, 1)
self.assertTrue(result.stderr, "Expected warning output when lock contention occurs")
def test_daily_dnr_fails_on_sqlite_error(self):
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
# Point DB at a directory to provoke a sqlite3.Error when connecting
db = td_path / "not_a_file.sqlite"
db.mkdir(parents=True, exist_ok=True)
log_dir = td_path / "logs"
marker_dir = td_path / "state/dnr"
lock_file = td_path / "state/dnr.lock"
env = os.environ.copy()
env["DAILY_DNR_DB_PATH"] = str(db)
env["DAILY_DNR_LOG_DIR"] = str(log_dir)
env["DAILY_DNR_MARKER_DIR"] = str(marker_dir)
env["DAILY_DNR_LOCK_FILE"] = str(lock_file)
result = subprocess.run(
[sys.executable, str(DAILY_DNR)],
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
# FAIL: sqlite3.Error paths should exit 2 and log an error
self.assertEqual(result.returncode, 2)
self.assertTrue(result.stderr, "Expected error output when sqlite3.Error is raised")
```
1. Ensure `DAILY_DNR` is the correct path constant for `daily_dnr_run.py`; if a different constant already exists in this file (e.g., `DAILY_DNR_RUN`), reuse it instead of introducing a new one.
2. Align the environment variable names (`DAILY_DNR_DB_PATH`, `DAILY_DNR_LOG_DIR`, `DAILY_DNR_MARKER_DIR`, `DAILY_DNR_LOCK_FILE`) with what `daily_dnr_run.main()` actually reads; adjust the keys in all four tests accordingly.
3. If `daily_dnr_run.py` is invoked directly as an executable script (shebang + `chmod +x`), and other tests call it without `sys.executable`, update the `subprocess.run` invocations to match the existing convention.
4. If the warning/error messages are known and stable, strengthen the `stderr` assertions to check for specific substrings (e.g., "WARN" or the concrete log message) rather than only non-empty output.
</issue_to_address>
### Comment 6
<location> `tests/runtime/test_runtime_tools.py:53-62` </location>
<code_context>
+ def test_health_daemon_writes_trend(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Extend health daemon tests to validate event contents, thresholds, and recovery behavior
The current test only asserts that a single FAIL cycle produces `runtime_health_trend.json` and the unhealthy marker. To better exercise the daemon’s behavior, consider:
- Asserting that the JSON trend entry includes the expected fields (`exit_code`, `state`, `consecutive_failures`, and optionally `event == 'state_changed'`).
- Adding a multi-cycle test with `max-cycles > 1` to verify that `threshold` is honored (e.g., threshold=2 only marks unhealthy after 2 consecutive FAILs).
- Adding a recovery test where a FAIL is followed by a non-FAIL cycle, asserting that `consecutive_failures` resets and any existing `system_unhealthy.marker` is removed.
This would more fully validate the long-running watchdog semantics.
Suggested implementation:
```python
import os
import json
import subprocess
import tempfile
from pathlib import Path
```
```python
def test_health_daemon_writes_trend(self):
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
state_dir = td_path / "state"
state_dir.mkdir(parents=True, exist_ok=True)
log_dir = td_path / "logs"
log_dir.mkdir(parents=True, exist_ok=True)
log_file = log_dir / "daemon.log"
# Force the daemon into a failing state so we exercise the
# unhealthy path and trend writing.
env = os.environ.copy()
env["DISK_PATH"] = "/path/does/not/exist"
cmd = [
"python3",
str(DAEMON),
"--state-dir",
str(state_dir),
"--log-file",
str(log_file),
"--max-cycles",
"1",
"--threshold",
"1",
]
result = subprocess.run(
cmd,
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
self.assertEqual(result.returncode, 0, msg=result.stderr)
trend_path = state_dir / "runtime_health_trend.json"
self.assertTrue(trend_path.exists())
# Validate that at least one event was written and that it contains
# the core fields we expect for watchdog semantics.
with trend_path.open() as f:
events = [json.loads(line) for line in f if line.strip()]
self.assertGreaterEqual(len(events), 1)
last = events[-1]
self.assertIn("exit_code", last)
self.assertIn("state", last)
self.assertIn("consecutive_failures", last)
if "event" in last:
self.assertEqual(last["event"], "state_changed")
marker_path = state_dir / "system_unhealthy.marker"
self.assertTrue(marker_path.exists())
def test_health_daemon_honors_failure_threshold(self):
"""Verify that threshold > 1 only marks unhealthy after enough consecutive failures."""
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
state_dir = td_path / "state"
state_dir.mkdir(parents=True, exist_ok=True)
log_dir = td_path / "logs"
log_dir.mkdir(parents=True, exist_ok=True)
log_file = log_dir / "daemon.log"
env = os.environ.copy()
env["DISK_PATH"] = "/path/does/not/exist"
cmd = [
"python3",
str(DAEMON),
"--state-dir",
str(state_dir),
"--log-file",
str(log_file),
"--max-cycles",
"3",
"--threshold",
"2",
]
result = subprocess.run(
cmd,
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
self.assertEqual(result.returncode, 0, msg=result.stderr)
trend_path = state_dir / "runtime_health_trend.json"
self.assertTrue(trend_path.exists())
with trend_path.open() as f:
events = [json.loads(line) for line in f if line.strip()]
# We expect multiple cycles to have been recorded.
self.assertGreaterEqual(len(events), 2)
# Find the last event for the failing probe and assert that the
# consecutive failure count reached at least the threshold.
last = events[-1]
self.assertIn("consecutive_failures", last)
self.assertGreaterEqual(last["consecutive_failures"], 2)
# Once threshold is reached, the system must be marked unhealthy.
marker_path = state_dir / "system_unhealthy.marker"
self.assertTrue(marker_path.exists())
def test_health_daemon_recovers_and_resets_failures(self):
"""Verify that a recovery cycle resets consecutive_failures and clears the unhealthy marker."""
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
state_dir = td_path / "state"
state_dir.mkdir(parents=True, exist_ok=True)
log_dir = td_path / "logs"
log_dir.mkdir(parents=True, exist_ok=True)
log_file = log_dir / "daemon.log"
# First run: force failures so we cross the threshold.
env_fail = os.environ.copy()
env_fail["DISK_PATH"] = "/path/does/not/exist"
cmd = [
"python3",
str(DAEMON),
"--state-dir",
str(state_dir),
"--log-file",
str(log_file),
"--max-cycles",
"3",
"--threshold",
"2",
]
result_fail = subprocess.run(
cmd,
cwd=ROOT,
env=env_fail,
capture_output=True,
text=True,
)
self.assertEqual(result_fail.returncode, 0, msg=result_fail.stderr)
marker_path = state_dir / "system_unhealthy.marker"
self.assertTrue(marker_path.exists())
# Second run: allow the daemon to succeed so it can recover.
env_ok = os.environ.copy()
# Do not override DISK_PATH (or set it to a valid path) so the probe passes.
result_ok = subprocess.run(
cmd,
cwd=ROOT,
env=env_ok,
capture_output=True,
text=True,
)
self.assertEqual(result_ok.returncode, 0, msg=result_ok.stderr)
trend_path = state_dir / "runtime_health_trend.json"
self.assertTrue(trend_path.exists())
with trend_path.open() as f:
events = [json.loads(line) for line in f if line.strip()]
self.assertGreaterEqual(len(events), 1)
last = events[-1]
# Recovery should reset the failure counter.
self.assertIn("consecutive_failures", last)
self.assertEqual(last["consecutive_failures"], 0)
# Recovery should also clear any unhealthy marker.
self.assertFalse(marker_path.exists())
```
The commands and JSON field names in these tests assume the daemon supports the following CLI options and event schema:
- CLI flags: `--state-dir`, `--log-file`, `--max-cycles`, `--threshold`.
- JSON event fields: at least `exit_code`, `state`, `consecutive_failures`, and optionally `event == "state_changed"` on state transitions.
- A marker file named `system_unhealthy.marker` created/removed under `state_dir`.
If the actual CLI or schema differ, you should:
1. Adjust the `cmd` array flags/values to match the real arguments.
2. Update the asserted JSON keys (`exit_code`, `state`, `consecutive_failures`, `event`) to the actual field names used by your daemon.
3. Update the marker filename (`system_unhealthy.marker`) and its lifecycle expectations (when it is created/removed) to match the real implementation.
4. If the recovery behavior uses a different mechanism than resetting `consecutive_failures` to `0`, adapt the final assertions accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+131
to
+148
| def run_preflight_gate() -> int: | ||
| if os.environ.get("SKIP_PREFLIGHT", "0") == "1": | ||
| return 0 | ||
|
|
||
| try: | ||
| result = subprocess.run( | ||
| ["./tools/runtime/preflight.sh", "--summary"], | ||
| capture_output=True, | ||
| text=True, | ||
| check=False, | ||
| ) | ||
| if result.stdout: | ||
| print(result.stdout.strip()) | ||
| if result.stderr: | ||
| print(result.stderr.strip()) | ||
| return int(result.returncode) | ||
| except Exception: | ||
| return 1 |
There was a problem hiding this comment.
suggestion: Hãy cân nhắc log khi chính việc gọi preflight bị lỗi thay vì âm thầm trả về 1.
Hiện tại mọi ngoại lệ từ subprocess.run (thiếu script, quyền truy cập, v.v.) đều được ánh xạ thành mã thoát 1 mà không có ngữ cảnh, khiến lỗi hạ tầng không phân biệt được với một WARN thực sự. Vui lòng log ngoại lệ hoặc một thông báo lỗi ngắn trước khi trả về để bên gọi có thể phân biệt lỗi preflight với lỗi runtime/môi trường khi diễn giải mã thoát.
Suggested change
| def run_preflight_gate() -> int: | |
| if os.environ.get("SKIP_PREFLIGHT", "0") == "1": | |
| return 0 | |
| try: | |
| result = subprocess.run( | |
| ["./tools/runtime/preflight.sh", "--summary"], | |
| capture_output=True, | |
| text=True, | |
| check=False, | |
| ) | |
| if result.stdout: | |
| print(result.stdout.strip()) | |
| if result.stderr: | |
| print(result.stderr.strip()) | |
| return int(result.returncode) | |
| except Exception: | |
| return 1 | |
| def run_preflight_gate() -> int: | |
| if os.environ.get("SKIP_PREFLIGHT", "0") == "1": | |
| return 0 | |
| try: | |
| result = subprocess.run( | |
| ["./tools/runtime/preflight.sh", "--summary"], | |
| capture_output=True, | |
| text=True, | |
| check=False, | |
| ) | |
| if result.stdout: | |
| print(result.stdout.strip()) | |
| if result.stderr: | |
| print(result.stderr.strip()) | |
| return int(result.returncode) | |
| except Exception: | |
| logging.exception("Preflight gate invocation failed (unable to run preflight.sh)") | |
| return 1 |
Original comment in English
suggestion: Consider logging when preflight invocation itself fails instead of silently returning 1.
Right now any exception from subprocess.run (missing script, permissions, etc.) is mapped to exit code 1 with no context, making infra failures indistinguishable from a real WARN. Please log the exception or a short error message before returning so callers can differentiate preflight failures from runtime/environment issues when interpreting the exit code.
Suggested change
| def run_preflight_gate() -> int: | |
| if os.environ.get("SKIP_PREFLIGHT", "0") == "1": | |
| return 0 | |
| try: | |
| result = subprocess.run( | |
| ["./tools/runtime/preflight.sh", "--summary"], | |
| capture_output=True, | |
| text=True, | |
| check=False, | |
| ) | |
| if result.stdout: | |
| print(result.stdout.strip()) | |
| if result.stderr: | |
| print(result.stderr.strip()) | |
| return int(result.returncode) | |
| except Exception: | |
| return 1 | |
| def run_preflight_gate() -> int: | |
| if os.environ.get("SKIP_PREFLIGHT", "0") == "1": | |
| return 0 | |
| try: | |
| result = subprocess.run( | |
| ["./tools/runtime/preflight.sh", "--summary"], | |
| capture_output=True, | |
| text=True, | |
| check=False, | |
| ) | |
| if result.stdout: | |
| print(result.stdout.strip()) | |
| if result.stderr: | |
| print(result.stderr.strip()) | |
| return int(result.returncode) | |
| except Exception: | |
| logging.exception("Preflight gate invocation failed (unable to run preflight.sh)") | |
| return 1 |
Comment on lines
+36
to
+37
| def run_preflight() -> int: | ||
| result = subprocess.run(["./tools/runtime/preflight.sh", "--summary"], capture_output=True, text=True) |
There was a problem hiding this comment.
issue (bug_risk): Việc dùng đường dẫn tương đối cho preflight.sh khiến daemon phụ thuộc vào thư mục làm việc hiện tại.
`subprocess.run([
Original comment in English
issue (bug_risk): Using a relative path for preflight.sh makes the daemon sensitive to the working directory.
`subprocess.run([
| set -euo pipefail | ||
|
|
||
| MIN_FREE_GB="${MIN_FREE_GB:-15}" | ||
| DISK_PATH="${DISK_PATH:-/Users/andy}" |
There was a problem hiding this comment.
suggestion (bug_risk): Giá trị mặc định của DISK_PATH gắn với người dùng cụ thể và có thể không chạy được trên các máy khác.
Hãy cân nhắc đặt mặc định thành một đường dẫn tổng quát hơn (ví dụ ${DISK_PATH:-$HOME} hoặc tương tự) để preflight có thể dùng lại được, đồng thời vẫn cho phép override qua biến môi trường.
Suggested change
| DISK_PATH="${DISK_PATH:-/Users/andy}" | |
| DISK_PATH="${DISK_PATH:-$HOME}" |
Original comment in English
suggestion (bug_risk): The default DISK_PATH is user-specific and may not work on other machines.
Consider defaulting to a more generic path (e.g., ${DISK_PATH:-$HOME} or similar) so the preflight remains portable while still allowing environment overrides.
Suggested change
| DISK_PATH="${DISK_PATH:-/Users/andy}" | |
| DISK_PATH="${DISK_PATH:-$HOME}" |
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
|
|
||
| SCRIPT_PATH="${1:-${ORIG:-$HOME/workbench/daily_dnr_run.py}}" |
There was a problem hiding this comment.
suggestion (bug_risk): Giá trị mặc định của SCRIPT_PATH trỏ tới một đường dẫn gần như chỉ tồn tại cục bộ, có thể không ổn định.
Vì $HOME/workbench/daily_dnr_run.py phụ thuộc vào môi trường, nên khi ORIG không được set và không truyền tham số vị trí nào, script này rất có thể sẽ lỗi trong hầu hết môi trường. Hãy cân nhắc đặt mặc định thành một đường dẫn bên trong repo (ví dụ ./tools/runtime/daily_dnr_run.py được resolve tương đối so với wrapper) để script có một giá trị mặc định có thể dùng lại và có thể dự đoán được.
Suggested change
| SCRIPT_PATH="${1:-${ORIG:-$HOME/workbench/daily_dnr_run.py}}" | |
| SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" >/dev/null 2>&1 && pwd)" | |
| SCRIPT_PATH="${1:-${ORIG:-"$SCRIPT_DIR/daily_dnr_run.py"}}" |
Original comment in English
suggestion (bug_risk): Default SCRIPT_PATH points to a likely local-only path, which may be brittle.
Because $HOME/workbench/daily_dnr_run.py is environment-specific, when ORIG is unset and no positional argument is provided this will likely break in most environments. Consider defaulting to a path within the repo (e.g., ./tools/runtime/daily_dnr_run.py resolved relative to the wrapper) so the script has a portable, predictable default.
Suggested change
| SCRIPT_PATH="${1:-${ORIG:-$HOME/workbench/daily_dnr_run.py}}" | |
| SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" >/dev/null 2>&1 && pwd)" | |
| SCRIPT_PATH="${1:-${ORIG:-"$SCRIPT_DIR/daily_dnr_run.py"}}" |
Comment on lines
+22
to
+31
| def test_daily_dnr_idempotent_same_day(self): | ||
| with tempfile.TemporaryDirectory() as td: | ||
| td_path = Path(td) | ||
| db = td_path / "memory.sqlite" | ||
| log_dir = td_path / "logs" | ||
| marker_dir = td_path / "state/dnr" | ||
| lock_file = td_path / "state/dnr.lock" | ||
| db.parent.mkdir(parents=True, exist_ok=True) | ||
| sqlite3.connect(db).close() | ||
|
|
There was a problem hiding this comment.
suggestion (testing): Thêm các bài test DNR cho trường hợp thiếu DB, tranh chấp lock, và các nhánh lỗi SQLite
Để kiểm thử đầy đủ hợp đồng mã thoát của daily_dnr_run.py, vui lòng thêm các bài test:
- Bao phủ trường hợp thiếu đường dẫn DB và assert WARN (thoát 1) cùng log cảnh báo.
- Mô phỏng tranh chấp lock (giữ trước file lock) và assert nhánh
RuntimeErrorcũng cho kết quả WARN (thoát 1). - Gây ra
sqlite3.Error(ví dụ file DB hỏng hoặc lỗi quyền) và assert FAIL (thoát 2).
Điều này đảm bảo tất cả các nhánh được mô tả trongmain()đều được kiểm chứng bằng test.
Gợi ý triển khai:
DAEMON = ROOT / "tools/runtime/runtime_health_daemon.py"
DAILY_DNR = ROOT / "tools/runtime/daily_dnr_run.py" def test_preflight_fail_on_invalid_disk_path(self):
env = os.environ.copy()
env["DISK_PATH"] = "/path/does/not/exist"
result = subprocess.run([str(PREFLIGHT), "--summary"], cwd=ROOT, env=env, capture_output=True, text=True)
self.assertEqual(result.returncode, 2)
def test_daily_dnr_idempotent_same_day(self):
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
db = td_path / "memory.sqlite"
log_dir = td_path / "logs"
marker_dir = td_path / "state/dnr"
lock_file = td_path / "state/dnr.lock"
db.parent.mkdir(parents=True, exist_ok=True)
sqlite3.connect(db).close()
env = os.environ.copy()
env["DAILY_DNR_DB_PATH"] = str(db)
env["DAILY_DNR_LOG_DIR"] = str(log_dir)
env["DAILY_DNR_MARKER_DIR"] = str(marker_dir)
env["DAILY_DNR_LOCK_FILE"] = str(lock_file)
# First run should succeed and create the marker/lock state
result1 = subprocess.run(
[sys.executable, str(DAILY_DNR)],
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
self.assertEqual(result1.returncode, 0)
# Second run on the same day should be idempotent and still succeed
result2 = subprocess.run(
[sys.executable, str(DAILY_DNR)],
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
self.assertEqual(result2.returncode, 0)
def test_daily_dnr_warns_on_missing_db(self):
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
db = td_path / "missing.sqlite"
log_dir = td_path / "logs"
marker_dir = td_path / "state/dnr"
lock_file = td_path / "state/dnr.lock"
env = os.environ.copy()
env["DAILY_DNR_DB_PATH"] = str(db)
env["DAILY_DNR_LOG_DIR"] = str(log_dir)
env["DAILY_DNR_MARKER_DIR"] = str(marker_dir)
env["DAILY_DNR_LOCK_FILE"] = str(lock_file)
result = subprocess.run(
[sys.executable, str(DAILY_DNR)],
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
# WARN: Missing DB should exit 1 and emit a warning on stderr
self.assertEqual(result.returncode, 1)
self.assertTrue(result.stderr, "Expected warning output when DB path is missing")
def test_daily_dnr_warns_on_lock_contention(self):
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
db = td_path / "memory.sqlite"
log_dir = td_path / "logs"
marker_dir = td_path / "state/dnr"
lock_file = td_path / "state/dnr.lock"
db.parent.mkdir(parents=True, exist_ok=True)
sqlite3.connect(db).close()
env = os.environ.copy()
env["DAILY_DNR_DB_PATH"] = str(db)
env["DAILY_DNR_LOG_DIR"] = str(log_dir)
env["DAILY_DNR_MARKER_DIR"] = str(marker_dir)
env["DAILY_DNR_LOCK_FILE"] = str(lock_file)
# Pre-hold the lock file to simulate another process owning it
lock_file.parent.mkdir(parents=True, exist_ok=True)
with open(lock_file, "w") as fh:
try:
import fcntl
fcntl.flock(fh, fcntl.LOCK_EX | fcntl.LOCK_NB)
except (ImportError, OSError):
# If flock is not available or locking fails for platform reasons,
# still keep the file present to exercise the lock contention path
pass
result = subprocess.run(
[sys.executable, str(DAILY_DNR)],
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
# WARN: lock contention is treated as RuntimeError -> exit 1
self.assertEqual(result.returncode, 1)
self.assertTrue(result.stderr, "Expected warning output when lock contention occurs")
def test_daily_dnr_fails_on_sqlite_error(self):
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
# Point DB at a directory to provoke a sqlite3.Error when connecting
db = td_path / "not_a_file.sqlite"
db.mkdir(parents=True, exist_ok=True)
log_dir = td_path / "logs"
marker_dir = td_path / "state/dnr"
lock_file = td_path / "state/dnr.lock"
env = os.environ.copy()
env["DAILY_DNR_DB_PATH"] = str(db)
env["DAILY_DNR_LOG_DIR"] = str(log_dir)
env["DAILY_DNR_MARKER_DIR"] = str(marker_dir)
env["DAILY_DNR_LOCK_FILE"] = str(lock_file)
result = subprocess.run(
[sys.executable, str(DAILY_DNR)],
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
# FAIL: sqlite3.Error paths should exit 2 and log an error
self.assertEqual(result.returncode, 2)
self.assertTrue(result.stderr, "Expected error output when sqlite3.Error is raised")- Đảm bảo
DAILY_DNRlà hằng đường dẫn đúng chodaily_dnr_run.py; nếu file này đã có một hằng khác (ví dụDAILY_DNR_RUN), hãy tái sử dụng thay vì tạo hằng mới. - Đồng bộ tên các biến môi trường (
DAILY_DNR_DB_PATH,DAILY_DNR_LOG_DIR,DAILY_DNR_MARKER_DIR,DAILY_DNR_LOCK_FILE) với những gìdaily_dnr_run.main()thực sự đọc; cập nhật các key trong cả bốn bài test cho phù hợp. - Nếu
daily_dnr_run.pyđược gọi trực tiếp như một script thực thi (shebang +chmod +x), và các test khác gọi nó mà không quasys.executable, hãy cập nhật các lệnhsubprocess.runcho phù hợp với convention hiện tại. - Nếu thông báo cảnh báo/lỗi đã biết và ổn định, hãy tăng độ chặt cho các assert trên
stderrbằng cách kiểm tra các substring cụ thể (ví dụ "WARN" hoặc thông điệp log cụ thể) thay vì chỉ kiểm tra là có output.
Original comment in English
suggestion (testing): Add DNR tests for missing DB, lock contention, and SQLite error paths
To fully exercise daily_dnr_run.py’s exit contract, please add tests that:
- Cover a missing DB path and assert WARN (exit 1) plus the warning log.
- Simulate lock contention (pre-hold the lock file) and assert the
RuntimeErrorpath also yields WARN (exit 1). - Trigger a
sqlite3.Error(e.g., bad DB file or permissions) and assert FAIL (exit 2).
This will ensure all documented branches inmain()are verified by tests.
Suggested implementation:
DAEMON = ROOT / "tools/runtime/runtime_health_daemon.py"
DAILY_DNR = ROOT / "tools/runtime/daily_dnr_run.py" def test_preflight_fail_on_invalid_disk_path(self):
env = os.environ.copy()
env["DISK_PATH"] = "/path/does/not/exist"
result = subprocess.run([str(PREFLIGHT), "--summary"], cwd=ROOT, env=env, capture_output=True, text=True)
self.assertEqual(result.returncode, 2)
def test_daily_dnr_idempotent_same_day(self):
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
db = td_path / "memory.sqlite"
log_dir = td_path / "logs"
marker_dir = td_path / "state/dnr"
lock_file = td_path / "state/dnr.lock"
db.parent.mkdir(parents=True, exist_ok=True)
sqlite3.connect(db).close()
env = os.environ.copy()
env["DAILY_DNR_DB_PATH"] = str(db)
env["DAILY_DNR_LOG_DIR"] = str(log_dir)
env["DAILY_DNR_MARKER_DIR"] = str(marker_dir)
env["DAILY_DNR_LOCK_FILE"] = str(lock_file)
# First run should succeed and create the marker/lock state
result1 = subprocess.run(
[sys.executable, str(DAILY_DNR)],
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
self.assertEqual(result1.returncode, 0)
# Second run on the same day should be idempotent and still succeed
result2 = subprocess.run(
[sys.executable, str(DAILY_DNR)],
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
self.assertEqual(result2.returncode, 0)
def test_daily_dnr_warns_on_missing_db(self):
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
db = td_path / "missing.sqlite"
log_dir = td_path / "logs"
marker_dir = td_path / "state/dnr"
lock_file = td_path / "state/dnr.lock"
env = os.environ.copy()
env["DAILY_DNR_DB_PATH"] = str(db)
env["DAILY_DNR_LOG_DIR"] = str(log_dir)
env["DAILY_DNR_MARKER_DIR"] = str(marker_dir)
env["DAILY_DNR_LOCK_FILE"] = str(lock_file)
result = subprocess.run(
[sys.executable, str(DAILY_DNR)],
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
# WARN: Missing DB should exit 1 and emit a warning on stderr
self.assertEqual(result.returncode, 1)
self.assertTrue(result.stderr, "Expected warning output when DB path is missing")
def test_daily_dnr_warns_on_lock_contention(self):
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
db = td_path / "memory.sqlite"
log_dir = td_path / "logs"
marker_dir = td_path / "state/dnr"
lock_file = td_path / "state/dnr.lock"
db.parent.mkdir(parents=True, exist_ok=True)
sqlite3.connect(db).close()
env = os.environ.copy()
env["DAILY_DNR_DB_PATH"] = str(db)
env["DAILY_DNR_LOG_DIR"] = str(log_dir)
env["DAILY_DNR_MARKER_DIR"] = str(marker_dir)
env["DAILY_DNR_LOCK_FILE"] = str(lock_file)
# Pre-hold the lock file to simulate another process owning it
lock_file.parent.mkdir(parents=True, exist_ok=True)
with open(lock_file, "w") as fh:
try:
import fcntl
fcntl.flock(fh, fcntl.LOCK_EX | fcntl.LOCK_NB)
except (ImportError, OSError):
# If flock is not available or locking fails for platform reasons,
# still keep the file present to exercise the lock contention path
pass
result = subprocess.run(
[sys.executable, str(DAILY_DNR)],
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
# WARN: lock contention is treated as RuntimeError -> exit 1
self.assertEqual(result.returncode, 1)
self.assertTrue(result.stderr, "Expected warning output when lock contention occurs")
def test_daily_dnr_fails_on_sqlite_error(self):
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
# Point DB at a directory to provoke a sqlite3.Error when connecting
db = td_path / "not_a_file.sqlite"
db.mkdir(parents=True, exist_ok=True)
log_dir = td_path / "logs"
marker_dir = td_path / "state/dnr"
lock_file = td_path / "state/dnr.lock"
env = os.environ.copy()
env["DAILY_DNR_DB_PATH"] = str(db)
env["DAILY_DNR_LOG_DIR"] = str(log_dir)
env["DAILY_DNR_MARKER_DIR"] = str(marker_dir)
env["DAILY_DNR_LOCK_FILE"] = str(lock_file)
result = subprocess.run(
[sys.executable, str(DAILY_DNR)],
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
# FAIL: sqlite3.Error paths should exit 2 and log an error
self.assertEqual(result.returncode, 2)
self.assertTrue(result.stderr, "Expected error output when sqlite3.Error is raised")- Ensure
DAILY_DNRis the correct path constant fordaily_dnr_run.py; if a different constant already exists in this file (e.g.,DAILY_DNR_RUN), reuse it instead of introducing a new one. - Align the environment variable names (
DAILY_DNR_DB_PATH,DAILY_DNR_LOG_DIR,DAILY_DNR_MARKER_DIR,DAILY_DNR_LOCK_FILE) with whatdaily_dnr_run.main()actually reads; adjust the keys in all four tests accordingly. - If
daily_dnr_run.pyis invoked directly as an executable script (shebang +chmod +x), and other tests call it withoutsys.executable, update thesubprocess.runinvocations to match the existing convention. - If the warning/error messages are known and stable, strengthen the
stderrassertions to check for specific substrings (e.g., "WARN" or the concrete log message) rather than only non-empty output.
Comment on lines
+53
to
+62
| def test_health_daemon_writes_trend(self): | ||
| with tempfile.TemporaryDirectory() as td: | ||
| td_path = Path(td) | ||
| state_dir = td_path / "state" | ||
| log_file = td_path / "logs/daemon.log" | ||
| env = os.environ.copy() | ||
| env["DISK_PATH"] = "/path/does/not/exist" | ||
|
|
||
| cmd = [ | ||
| "python3", |
There was a problem hiding this comment.
suggestion (testing): Mở rộng các test cho health daemon để kiểm tra nội dung sự kiện, ngưỡng và hành vi phục hồi
Bài test hiện tại chỉ assert rằng một chu kỳ FAIL đơn lẻ tạo ra runtime_health_trend.json và marker unhealthy. Để kiểm thử hành vi của daemon tốt hơn, hãy cân nhắc:
- Assert rằng entry JSON trong trend chứa các trường mong đợi (
exit_code,state,consecutive_failures, và tùy chọnevent == 'state_changed'). - Thêm một bài test đa chu kỳ với
max-cycles > 1để xác minhthresholdđược tôn trọng (ví dụ threshold=2 chỉ đánh dấu unhealthy sau 2 FAIL liên tiếp). - Thêm một bài test phục hồi, trong đó một chu kỳ FAIL được theo sau bởi một chu kỳ không FAIL, assert rằng
consecutive_failuresđược reset và bất kỳsystem_unhealthy.markerhiện có nào được xóa.
Điều này sẽ giúp kiểm chứng đầy đủ ngữ nghĩa watchdog chạy lâu dài.
Gợi ý triển khai:
import os
import json
import subprocess
import tempfile
from pathlib import Path def test_health_daemon_writes_trend(self):
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
state_dir = td_path / "state"
state_dir.mkdir(parents=True, exist_ok=True)
log_dir = td_path / "logs"
log_dir.mkdir(parents=True, exist_ok=True)
log_file = log_dir / "daemon.log"
# Force the daemon into a failing state so we exercise the
# unhealthy path and trend writing.
env = os.environ.copy()
env["DISK_PATH"] = "/path/does/not/exist"
cmd = [
"python3",
str(DAEMON),
"--state-dir",
str(state_dir),
"--log-file",
str(log_file),
"--max-cycles",
"1",
"--threshold",
"1",
]
result = subprocess.run(
cmd,
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
self.assertEqual(result.returncode, 0, msg=result.stderr)
trend_path = state_dir / "runtime_health_trend.json"
self.assertTrue(trend_path.exists())
# Validate that at least one event was written and that it contains
# the core fields we expect for watchdog semantics.
with trend_path.open() as f:
events = [json.loads(line) for line in f if line.strip()]
self.assertGreaterEqual(len(events), 1)
last = events[-1]
self.assertIn("exit_code", last)
self.assertIn("state", last)
self.assertIn("consecutive_failures", last)
if "event" in last:
self.assertEqual(last["event"], "state_changed")
marker_path = state_dir / "system_unhealthy.marker"
self.assertTrue(marker_path.exists())
def test_health_daemon_honors_failure_threshold(self):
"""Verify that threshold > 1 only marks unhealthy after enough consecutive failures."""
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
state_dir = td_path / "state"
state_dir.mkdir(parents=True, exist_ok=True)
log_dir = td_path / "logs"
log_dir.mkdir(parents=True, exist_ok=True)
log_file = log_dir / "daemon.log"
env = os.environ.copy()
env["DISK_PATH"] = "/path/does/not/exist"
cmd = [
"python3",
str(DAEMON),
"--state-dir",
str(state_dir),
"--log-file",
str(log_file),
"--max-cycles",
"3",
"--threshold",
"2",
]
result = subprocess.run(
cmd,
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
self.assertEqual(result.returncode, 0, msg=result.stderr)
trend_path = state_dir / "runtime_health_trend.json"
self.assertTrue(trend_path.exists())
with trend_path.open() as f:
events = [json.loads(line) for line in f if line.strip()]
# We expect multiple cycles to have been recorded.
self.assertGreaterEqual(len(events), 2)
# Find the last event for the failing probe and assert that the
# consecutive failure count reached at least the threshold.
last = events[-1]
self.assertIn("consecutive_failures", last)
self.assertGreaterEqual(last["consecutive_failures"], 2)
# Once threshold is reached, the system must be marked unhealthy.
marker_path = state_dir / "system_unhealthy.marker"
self.assertTrue(marker_path.exists())
def test_health_daemon_recovers_and_resets_failures(self):
"""Verify that a recovery cycle resets consecutive_failures and clears the unhealthy marker."""
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
state_dir = td_path / "state"
state_dir.mkdir(parents=True, exist_ok=True)
log_dir = td_path / "logs"
log_dir.mkdir(parents=True, exist_ok=True)
log_file = log_dir / "daemon.log"
# First run: force failures so we cross the threshold.
env_fail = os.environ.copy()
env_fail["DISK_PATH"] = "/path/does/not/exist"
cmd = [
"python3",
str(DAEMON),
"--state-dir",
str(state_dir),
"--log-file",
str(log_file),
"--max-cycles",
"3",
"--threshold",
"2",
]
result_fail = subprocess.run(
cmd,
cwd=ROOT,
env=env_fail,
capture_output=True,
text=True,
)
self.assertEqual(result_fail.returncode, 0, msg=result_fail.stderr)
marker_path = state_dir / "system_unhealthy.marker"
self.assertTrue(marker_path.exists())
# Second run: allow the daemon to succeed so it can recover.
env_ok = os.environ.copy()
# Do not override DISK_PATH (or set it to a valid path) so the probe passes.
result_ok = subprocess.run(
cmd,
cwd=ROOT,
env=env_ok,
capture_output=True,
text=True,
)
self.assertEqual(result_ok.returncode, 0, msg=result_ok.stderr)
trend_path = state_dir / "runtime_health_trend.json"
self.assertTrue(trend_path.exists())
with trend_path.open() as f:
events = [json.loads(line) for line in f if line.strip()]
self.assertGreaterEqual(len(events), 1)
last = events[-1]
# Recovery should reset the failure counter.
self.assertIn("consecutive_failures", last)
self.assertEqual(last["consecutive_failures"], 0)
# Recovery should also clear any unhealthy marker.
self.assertFalse(marker_path.exists())Các lệnh và tên trường JSON trong các bài test này giả định daemon hỗ trợ các tùy chọn CLI và schema sự kiện sau:
- Cờ CLI:
--state-dir,--log-file,--max-cycles,--threshold. - Các trường sự kiện JSON: ít nhất
exit_code,state,consecutive_failures, và tùy chọnevent == "state_changed"khi chuyển trạng thái. - Một file marker tên
system_unhealthy.markerđược tạo/xóa dướistate_dir.
Nếu CLI hoặc schema thực tế khác, bạn nên:
- Điều chỉnh mảng
cmd(các cờ/giá trị) cho khớp với các tham số thật. - Cập nhật các key JSON được assert (
exit_code,state,consecutive_failures,event) thành các tên trường thật sự mà daemon sử dụng. - Cập nhật tên file marker (
system_unhealthy.marker) và kỳ vọng về vòng đời của nó (khi nào được tạo/xóa) để khớp với triển khai thực tế. - Nếu hành vi phục hồi dùng cơ chế khác với việc reset
consecutive_failuresvề0, hãy điều chỉnh các assert cuối cùng cho phù hợp.
Original comment in English
suggestion (testing): Extend health daemon tests to validate event contents, thresholds, and recovery behavior
The current test only asserts that a single FAIL cycle produces runtime_health_trend.json and the unhealthy marker. To better exercise the daemon’s behavior, consider:
- Asserting that the JSON trend entry includes the expected fields (
exit_code,state,consecutive_failures, and optionallyevent == 'state_changed'). - Adding a multi-cycle test with
max-cycles > 1to verify thatthresholdis honored (e.g., threshold=2 only marks unhealthy after 2 consecutive FAILs). - Adding a recovery test where a FAIL is followed by a non-FAIL cycle, asserting that
consecutive_failuresresets and any existingsystem_unhealthy.markeris removed.
This would more fully validate the long-running watchdog semantics.
Suggested implementation:
import os
import json
import subprocess
import tempfile
from pathlib import Path def test_health_daemon_writes_trend(self):
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
state_dir = td_path / "state"
state_dir.mkdir(parents=True, exist_ok=True)
log_dir = td_path / "logs"
log_dir.mkdir(parents=True, exist_ok=True)
log_file = log_dir / "daemon.log"
# Force the daemon into a failing state so we exercise the
# unhealthy path and trend writing.
env = os.environ.copy()
env["DISK_PATH"] = "/path/does/not/exist"
cmd = [
"python3",
str(DAEMON),
"--state-dir",
str(state_dir),
"--log-file",
str(log_file),
"--max-cycles",
"1",
"--threshold",
"1",
]
result = subprocess.run(
cmd,
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
self.assertEqual(result.returncode, 0, msg=result.stderr)
trend_path = state_dir / "runtime_health_trend.json"
self.assertTrue(trend_path.exists())
# Validate that at least one event was written and that it contains
# the core fields we expect for watchdog semantics.
with trend_path.open() as f:
events = [json.loads(line) for line in f if line.strip()]
self.assertGreaterEqual(len(events), 1)
last = events[-1]
self.assertIn("exit_code", last)
self.assertIn("state", last)
self.assertIn("consecutive_failures", last)
if "event" in last:
self.assertEqual(last["event"], "state_changed")
marker_path = state_dir / "system_unhealthy.marker"
self.assertTrue(marker_path.exists())
def test_health_daemon_honors_failure_threshold(self):
"""Verify that threshold > 1 only marks unhealthy after enough consecutive failures."""
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
state_dir = td_path / "state"
state_dir.mkdir(parents=True, exist_ok=True)
log_dir = td_path / "logs"
log_dir.mkdir(parents=True, exist_ok=True)
log_file = log_dir / "daemon.log"
env = os.environ.copy()
env["DISK_PATH"] = "/path/does/not/exist"
cmd = [
"python3",
str(DAEMON),
"--state-dir",
str(state_dir),
"--log-file",
str(log_file),
"--max-cycles",
"3",
"--threshold",
"2",
]
result = subprocess.run(
cmd,
cwd=ROOT,
env=env,
capture_output=True,
text=True,
)
self.assertEqual(result.returncode, 0, msg=result.stderr)
trend_path = state_dir / "runtime_health_trend.json"
self.assertTrue(trend_path.exists())
with trend_path.open() as f:
events = [json.loads(line) for line in f if line.strip()]
# We expect multiple cycles to have been recorded.
self.assertGreaterEqual(len(events), 2)
# Find the last event for the failing probe and assert that the
# consecutive failure count reached at least the threshold.
last = events[-1]
self.assertIn("consecutive_failures", last)
self.assertGreaterEqual(last["consecutive_failures"], 2)
# Once threshold is reached, the system must be marked unhealthy.
marker_path = state_dir / "system_unhealthy.marker"
self.assertTrue(marker_path.exists())
def test_health_daemon_recovers_and_resets_failures(self):
"""Verify that a recovery cycle resets consecutive_failures and clears the unhealthy marker."""
with tempfile.TemporaryDirectory() as td:
td_path = Path(td)
state_dir = td_path / "state"
state_dir.mkdir(parents=True, exist_ok=True)
log_dir = td_path / "logs"
log_dir.mkdir(parents=True, exist_ok=True)
log_file = log_dir / "daemon.log"
# First run: force failures so we cross the threshold.
env_fail = os.environ.copy()
env_fail["DISK_PATH"] = "/path/does/not/exist"
cmd = [
"python3",
str(DAEMON),
"--state-dir",
str(state_dir),
"--log-file",
str(log_file),
"--max-cycles",
"3",
"--threshold",
"2",
]
result_fail = subprocess.run(
cmd,
cwd=ROOT,
env=env_fail,
capture_output=True,
text=True,
)
self.assertEqual(result_fail.returncode, 0, msg=result_fail.stderr)
marker_path = state_dir / "system_unhealthy.marker"
self.assertTrue(marker_path.exists())
# Second run: allow the daemon to succeed so it can recover.
env_ok = os.environ.copy()
# Do not override DISK_PATH (or set it to a valid path) so the probe passes.
result_ok = subprocess.run(
cmd,
cwd=ROOT,
env=env_ok,
capture_output=True,
text=True,
)
self.assertEqual(result_ok.returncode, 0, msg=result_ok.stderr)
trend_path = state_dir / "runtime_health_trend.json"
self.assertTrue(trend_path.exists())
with trend_path.open() as f:
events = [json.loads(line) for line in f if line.strip()]
self.assertGreaterEqual(len(events), 1)
last = events[-1]
# Recovery should reset the failure counter.
self.assertIn("consecutive_failures", last)
self.assertEqual(last["consecutive_failures"], 0)
# Recovery should also clear any unhealthy marker.
self.assertFalse(marker_path.exists())The commands and JSON field names in these tests assume the daemon supports the following CLI options and event schema:
- CLI flags:
--state-dir,--log-file,--max-cycles,--threshold. - JSON event fields: at least
exit_code,state,consecutive_failures, and optionallyevent == "state_changed"on state transitions. - A marker file named
system_unhealthy.markercreated/removed understate_dir.
If the actual CLI or schema differ, you should:
- Adjust the
cmdarray flags/values to match the real arguments. - Update the asserted JSON keys (
exit_code,state,consecutive_failures,event) to the actual field names used by your daemon. - Update the marker filename (
system_unhealthy.marker) and its lifecycle expectations (when it is created/removed) to match the real implementation. - If the recovery behavior uses a different mechanism than resetting
consecutive_failuresto0, adapt the final assertions accordingly.
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.
Motivation
2).0/1/2exit semantics across failure scenarios.Description
./tools/runtime/preflight.sh --summaryand immediately exiting onrc==2intools/runtime/launchd_safe_wrapper.shandtools/runtime/daily_dnr_run.py(the latter can be skipped withSKIP_PREFLIGHT=1).tools/runtime/bootstrap_runtime.shwhich runs the preflight gate (respectsPOLICY_ALLOW_WARN), ensures~/.hyperai/{logs,state,db}, initializes the SQLite DB withPRAGMA journal_mode=WALand baseline tables, runs log rotation and the daily DNR, and sequentially bootstraps launchd jobs with a final post-bootstrap preflight and deterministic exit code.tools/runtime/preflight.shwith acheck_db_integrityfunction that runssqlite3 "$DB_PATH" "PRAGMA integrity_check;"and maps results to0/1/2per policy, keeping the prior disk, entrypoint, job, and log gates and the--summarymode.tools/runtime/test_runtime_matrix.sh(simulates missing entrypoints, low disk, log pressure, corrupt DB, DB lock overlap, and launchd absence) andtests/runtime/test_runtime_tools.pyunit tests for preflight, DNR idempotence, and the health daemon.tools/runtime/runtime_health_daemon.pyto run preflight on an interval, log JSON event lines, track consecutive FAIL counts, and write ansystem_unhealthy.markerwhen threshold is reached; addedPROMPTS.mdand updateddocs/operations/runtime_stability_blueprint.mdto document the flow and contracts.Testing
bash -non modified shell scripts andpython3 -m py_compileon Python scripts and they succeeded.python3 -m unittest tests.runtime.test_runtime_toolswhich executed 3 tests and returnedOK../tools/runtime/test_runtime_matrix.shand it completed with no failures (all scenarios asserted expected exit semantics).POLICY_ALLOW_WARN=1bootstrap in a temporary environment to validate directory creation, DB initialization, DNR invocation, rotation, and observed the expected WARN-path with exit1under the constrained CI environment.Codex Task
Summary by Sourcery
Giới thiệu một luồng kiểm tra trước (preflight), khởi động (bootstrap) và giám sát sức khỏe runtime mang tính tất định cho HyperAI runtime cục bộ, được hỗ trợ bởi kiểm tra tính toàn vẹn SQLite, xoay vòng log (log rotation) và ma trận kiểm thử để áp đặt chặt chẽ ngữ nghĩa mã thoát 0/1/2.
Tính năng mới:
Cải tiến:
Tài liệu:
Kiểm thử:
Original summary in English
Summary by Sourcery
Introduce a deterministic runtime preflight, bootstrap, and health monitoring flow for the local HyperAI runtime, backed by SQLite integrity checks, log rotation, and a test matrix to enforce strict 0/1/2 exit semantics.
New Features:
Enhancements:
Documentation:
Tests: