jihoson
Reference in New Issue
Block a user
Delete Branch "feature/issue-396-397-runtime-anomaly-fixes"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
scripts/run_overnight.shstartup path with dashboard port preflight and startup grace liveness checksexecin the nohup shellscripts/runtime_verify_monitor.shdetect live runtime via process fallback (pgrep) even withoutrun_overnightpid filesTraceability
Validation
Issues
Code Review: PR #404 - Runtime Anomaly Fixes (#396, #397)
Strengths
find_live_pidsfallback 설계:pgrepself-exclusion 트릭([s]rc.main)으로 grep 자기 자신 매칭 방지. 단일 책임, 명확한 격리run_overnight.shL51-54 — 타임스탬프 로그 + non-zero exit. 이슈 #396 재현 증거와 정확히 대응kill -0으로 app_pid, watchdog_pid 양쪽 확인.STARTUP_GRACE_SEC환경변수로 CI 튜닝 가능MAX_LOOPSescape hatch: L10, L63-66 — 기본값 0(비활성). 벽시계 시간 없이 모니터 테스트 가능한 올바른 설계tmp_path격리,finally블록 정리. 3개 모두 통과dashboard_port스코프 수정: 이전에는if블록 안에서만 설정되어 외부APP_CMD제공 시 포트 체크에서 사용 불가했던 버그를 L12로 호이스팅해 수정Issues
Important (Should Fix)
1.
exec env $APP_CMD— 공백 포함 경로에서 word-splitting 취약run_overnight.shL57APP_CMD에 공백이 포함된 경로/인수가 있으면 잘못된 분리 발생VAR=value인라인 prefix를env VAR=val binary형식으로 분리하거나, "경로에 공백 불가" 가정을 명시적 주석으로 기록2.
is_port_in_use—ss명령 의존, portability 주장과 불일치run_overnight.shL37-40ss는 POSIX 아님 (iproute2필요). 커밋 메시지 "make overnight startup portable in CI environments"와 불일치ss없으면 silentlyfalse반환 → 포트 충돌 미감지nc -z또는/proc/net/tcpfallback 추가, 또는 커밋 메시지를 "Linux CI" 한정으로 수정3. 프로세스 alive 상태지만 run log 없을 때 false-positive
[ANOMALY]발생runtime_verify_monitor.shL101-113app_alive=1이고latest_run이 비어있을 때LIVE_MODE=PASS이지만, 이후 coverage 블록에서not_observed를 6 증가시켜[ANOMALY] coverage_not_observed=6로그 발생app_alive=1+latest_run비어있음 조건에서 coverage 6개 체크를DEFERRED reason=no_run_log_process_alive로 처리하거나 생략Minor (Nice to Have)
4. 모니터 테스트에
[ANOMALY]부재 assertion 미포함test_runtime_overnight_scripts.pyL51-52app_alive=1,LIVE_MODE=PASS확인하지만[ANOMALY]문자열 부재는 미검증assert "[ANOMALY]" not in log_text추가5. Grace-period 실패 경로 테스트 누락
test_runtime_overnight_scripts.pyL90-124APP_CMD="false",STARTUP_GRACE_SEC=1로 즉시 종료 케이스 추가,returncode != 0+"startup failed" in output검증6. 모니터에 포트 하드코딩 (
:8080)runtime_verify_monitor.shL82:ss -ltnp 2>/dev/null | rg -q ':8080'run_overnight.sh는 이제DASHBOARD_PORT변���를 사용하지만 모니터는 여전히 하드코딩DASHBOARD_PORT="${DASHBOARD_PORT:-8080}"추가 후 참조Assessment
머지 가능 여부: 조건부 Yes
핵심 수정 (포트 충돌 감지, grace-period liveness check, pid scoping 수정, 프로세스 기반 liveness fallback)은 모두 올바르게 구현되어 있고 테스트도 통과함. Important 이슈 #3(첫 run log 생성 전 false-positive ANOMALY)은 기능 자체를 깨진 않지만 운영 노이즈 발생 가능 — 머지 후 follow-up 이슈로 추적 권장.
Follow-up으로 추적할 항목: 이슈 #3 (false-positive anomaly), 이슈 #6 (하드코딩 포트)
리뷰 코멘트 확인 후 반영 완료.
반영 항목:
run_overnight.shis_port_in_use에ss -> lsof -> netstatfallback 추가APP_CMD문자열 처리 가정(공백 경로는 내부에서 quote 필요) 명시runtime_verify_monitor.shDASHBOARD_PORT환경변수화(8080 하드코딩 제거)app_alive=1 && run log 없음케이스에서 log-based coverage를DEFERRED로 처리[ANOMALY]false-positive 제거tests/test_runtime_overnight_scripts.py)[ANOMALY]미발생 검증 추가APP_CMD=false) 테스트 추가검증:
ruff check tests/test_runtime_overnight_scripts.pybash -n scripts/run_overnight.sh scripts/runtime_verify_monitor.shpytest -q tests/test_runtime_overnight_scripts.py(4 passed)Code Review (2차): PR #404 - 리뷰 반영 확인 (commit
3cde877)이전 이슈 처리 현황
Issue #1:
exec env $APP_CMDword-splitting 취약 — ⚠️ NOT ADDRESSED (주석만 추가)nohup bash -lc "exec env $APP_CMD"패턴 미변경Issue #2:
ss명령 의존성 — ✅ RESOLVED3단계 fallback 체인 도입:
netstat패턴이[:.]${port}로 Linux(0.0.0.0:8080)와 macOS(*.8080) 형식 양쪽 처리 — 정확한 구현Issue #3: false-positive
[ANOMALY]— ✅ RESOLVEDdefer_log_checks플래그 도입:app_alive=1+latest_run없음 →[INFO] run log not yet available; defer log-based coverage checksapp_alive=0+latest_run없음일 때만[ANOMALY]발생[ -z "$latest_run" ] && [ "$app_alive" -eq 0 ]와 동치인데defer_log_checks변수를 경유하고 있어 가독성이 살짝 낮으나 버그 아님)Issue #4:
[ANOMALY]부재 assertion — ✅ RESOLVED추가 확인.
Issue #5: Grace-period 실패 경로 테스트 — ✅ RESOLVED
test_run_overnight_fails_when_process_exits_before_grace_period추가:APP_CMD=false,STARTUP_GRACE_SEC=1— POSIX 즉시 종료 명령 사용으로 올바른 접근returncode != 0+"startup failed:" in output검증 — 스크립트 L84 메시지와 정확히 일치Issue #6: 포트 하드코딩 — ✅ RESOLVED
DASHBOARD_PORT="${DASHBOARD_PORT:-8080}"환경변수화:8080→:${DASHBOARD_PORT}\b(\bword boundary 추가로:18080오매칭 방지까지)신규 발견 이슈 (수정 과정에서 도입)
Minor
A.
runtime_verify_monitor.shL83 —ssfallback 미적용 (일관성 불일치)run_overnight.sh는 이번 수정으로ss → lsof → netstatfallback을 받았지만, 모니터 스크립트는 여전히:ss없는 환경에서port_alive가 silently 0 으로 유지됨. Issue #2 에서 적용한 portability 수정이 모니터에는 전파되지 않음.B. 새 테스트에서 watchdog 프로세스 누수
test_run_overnight_fails_when_process_exits_before_grace_period—APP_CMD=false로 스크립트가 exit 1로 종료되지만, watchdog은 grace period 체크 전에 이미nohup백그라운드 실행됨. 스크립트의startup failed경로가 watchdog을 kill하지 않아 테스트 종료 후 watchdog 프로세스가 잔류 (기본CHECK_INTERVAL동안). 프로덕션 로직 버그는 아니지만 테스트 환경 오염 우려.Assessment
머지 가능 여부: 조건부 Yes (Issue #1 팀 판단 후)
5개 이슈는 정확하게 수정되었고, 2개의 Minor 신규 이슈가 도입됨. 핵심 결정은 Issue #1: word-splitting 취약을 코드 수정 없이 주석 acknowledge만으로 close할지 여부. 나머지 신규 이슈(A, B)는 blocking 수준 아님.
추가 리뷰 코멘트 확인했고, 아래 항목을 코드로 반영 완료.
반영 내용:
run_overnight.shenv DASHBOARD_PORT=... "$PYTHON_BIN" ...)로 변경해 word-splitting 취약 최소화runtime_verify_monitor.shss -> lsof -> netstatfallback 함수(is_port_listening) 추가tests/test_runtime_overnight_scripts.py검증:
ruff check tests/test_runtime_overnight_scripts.pybash -n scripts/run_overnight.sh scripts/runtime_verify_monitor.shpytest -q tests/test_runtime_overnight_scripts.py(4 passed)추가 업데이트(
c412412) 기준 재리뷰 완료.결론:
검증:
ruff check tests/test_runtime_overnight_scripts.pybash -n scripts/run_overnight.sh scripts/runtime_verify_monitor.shpytest -q tests/test_runtime_overnight_scripts.py(4 passed)pytest -q tests/test_runtime_overnight_scripts.py::test_run_overnight_fails_when_process_exits_before_grace_period20회 반복 (PASS-20)참고 리스크(비차단):
APP_CMD경로는bash -lc문자열 파싱 계약을 유지하므로 공백/특수문자 포함 실행 경로는 호출 측 quoting이 필요함.추가 요청한 "반영" 항목 적용 완료.
변경사항:
run_overnight.shAPP_CMD_BIN+APP_CMD_ARGSRUNS_DASHBOARD플래그 기반으로 동작(기본/커스텀 공통)tests/test_runtime_overnight_scripts.pyAPP_CMD문자열 대신APP_CMD_BIN/APP_CMD_ARGS경로로 갱신호환성:
APP_CMD(문자열) 경로는 그대로 유지(하위 호환)APP_CMD_BIN/APP_CMD_ARGS검증:
bash -n scripts/run_overnight.shruff check tests/test_runtime_overnight_scripts.pypytest -q tests/test_runtime_overnight_scripts.py(4 passed)Code Review (3차): PR #404 - 2차 리뷰 반영 확인 (commits
c412412,dc0775c)이전 이슈 처리 현황
Issue #1:
exec env $APP_CMDword-splitting — ⚠️ PARTIAL신규
APP_CMD_BIN/APP_CMD_ARGS분리 인터페이스 도입으로 의미 있는 개선:"$APP_CMD_BIN"은 항상 quoted → 실행 파일 경로 공백 문제 해결"${app_args[@]}"로 인수 전달 → word-splitting 안전단, 기존 legacy 브랜치는 여전히 잔존:
이 경로는 주석만 있고 코드 수정 없음.
APP_CMD에 공백이 포함된 경로,$, 세미콜론 등 shell metacharacter가 있으면 여전히 위험.추가 주의사항 —
APP_CMD_ARGS배열 할당:의도적 word-split임을
shellcheck disable로 표시했으나,APP_CMD_ARGS="--config '/path with spaces/config.toml'"같은 내부 quoted 경로는 올바르게 처리되지 않음. 파일 어딘에도 이 제약이 문서화되지 않음 — 주석 추가 권장.결론: 새 인터페이스 사용 시 안전. legacy
APP_CMD경로는 "use at your own risk" escape hatch로 명시적 backward-compatibility 수단이 된 것으로 이해된다면 acceptable.Minor A:
runtime_verify_monitor.shL83ssfallback — ✅ RESOLVEDis_port_listening함수 도입 (L55-71):run_overnight.sh와 동일한 3단계 fallback. L101에서is_port_listening "$DASHBOARD_PORT"호출로 교체. 정확한 구현.(참고:
check_forbiddenL47에서rg는 여전히 fallback 없이 사용 중 — 이번 diff 범위 밖이지만 일관성 관점에서 언급)Minor B: 테스트에서 watchdog 프로세스 잔류 — ✅ RESOLVED
스크립트
startup failed경로에서 watchdog kill 추가 (L118):테스트에서 watchdog PID 파일 읽어
os.kill(watchdog_pid, 0)→ProcessLookupError확인으로 프로세스 종료 검증. 올바른 접근.사소한 관찰:
if watchdog_pid_file.exists()가드가 있어 PID 파일 없으면 assertion silently skip. 실제로 이 경로에서 PID 파일은 항상 존재하므로 practical 문제는 없지만, 엄격히는assert watchdog_pid_file.exists()를 먼저 두는 게 더 명확함신규 발견 이슈 (이번 수정에서 도입)
Minor
C. legacy
APP_CMD경로에서DASHBOARD_PORT암묵적 누락기존 코드는
APP_CMD문자열 자체에DASHBOARD_PORT=$dashboard_port를 포함했지만, 이번 리팩토링 후 default/safe 경로는env DASHBOARD_PORT="$dashboard_port"로 명시 주입, legacy 경로만 주입 없음:legacy
APP_CMD사용자가 스크립트가DASHBOARD_PORT를 주입해줄 것으로 기대했다면 silent behavioral change.$dashboard_port기본값8080이 환경변수 미설정 상태와 보통 일치하므로 실제 문제는 드물지만, legacy 경로 주석에 "DASHBOARD_PORT는 호출자가 직접 APP_CMD에 포함해야 함" 명시 권장.D.
APP_CMD_ARGS포맷 미문서화app_args=( $APP_CMD_ARGS )의 word-split 방식은 내부 공백이 있는 인수를 지원하지 않음. 스크립트 상단 변수 설명이나 사용 예시(e.g.,# APP_CMD_ARGS="--workers 4 --timeout 30")가 없어 내부 quoted 경로를 시도할 경우 조용히 깨짐.Overall Assessment
머지 가능 여부: Yes
Minor A와 B는 완전히 해소됨. Issue #1은 새 안전 인터페이스(
APP_CMD_BIN/APP_CMD_ARGS) 도입으로 실질적으로 개선됨 — legacy 경로는 명시적 backward-compatibility escape hatch로 남아있고 주석으로 제약이 문서화되어 있음. 신규 이슈 C, D는 모두 Minor 수준이며 blocking 아님. 팀이 legacyAPP_CMD경로를 "documented risk" 로 수용한다면 현재 상태로 머지 가능.최신 리뷰 재확인 완료.
리뷰에서 남은 minor C/D 항목(legacy APP_CMD 계약, APP_CMD_ARGS 포맷 제약 문서화)을
run_overnight.sh주석에 명시적으로 반영함.반영 커밋:
bcbbf80docs: clarify APP_CMD legacy and APP_CMD_ARGS contract핵심 명시:
APP_CMD_BIN+APP_CMD_ARGSAPP_CMD_ARGS는 shell word-splitting 기반이며 nested quote 보존 없음APP_CMD는 하위 호환 경로이며 quoting/env 주입은 호출자 책임--dashboard사용 시 비기본 포트가 필요하면DASHBOARD_PORT를 APP_CMD에 직접 포함해야 함Code Review (4차): PR #404 - 3차 리뷰 반영 확인 (commit
bcbbf80)이전 이슈 처리 현황
Minor C: legacy
APP_CMD경로에서DASHBOARD_PORT암묵적 누락 — ✅ RESOLVED스크립트 상단 contract 주석에 명시:
legacy 경로 사용자에게 DASHBOARD_PORT를 직접 포함해야 함을 명확히 고지함. 적절한 처리.
Minor D:
APP_CMD_ARGS포맷 미문서화 — ✅ RESOLVEDcontract 주석에 word-split 제약 명시:
내부 quoted 경로가 지원되지 않는다는 제약이 명확히 문서화됨. 적절한 처리.
신규 이슈
없음.
Overall Assessment
머지 가능 여부: Yes
6차 이슈 전체가 해소됨. Issue #1의 legacy
APP_CMD경로는 신규 안전 인터페이스(APP_CMD_BIN/APP_CMD_ARGS)로 대체되었고, 잔존하는 legacy 경로의 제약과 책임 소재가 contract 주석으로 명확히 문서화되어 있음. 전체 구현이 원래 이슈 #396, #397의 acceptance criteria를 충족하며 추가된 테스트도 통과함. 블로킹 이슈 없음.