Skip to content

fix(test): contain and reclaim Windows temporary roots - #4785

Closed
colthreepv wants to merge 1 commit into
lidge-jun:devfrom
colthreepv:codex/fix-windows-test-temp-cleanup
Closed

colthreepv wants to merge 1 commit into
lidge-jun:devfrom
colthreepv:codex/fix-windows-test-temp-cleanup

Conversation

@colthreepv

@colthreepv colthreepv commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Route TEMP, TMP, and TMPDIR for every test process into its per-run root, so nested test artifacts are removed with the owning run on both passing and failing paths.
  • Automatically reclaim Windows test roots older than 48 hours during ordinary test startup. Recovery is bounded and requires exact test-root names, direct TEMP containment, inactivity, no live owner PID, and a link-free tree; invalid ownership metadata fails closed.
  • Add regression coverage for wrapper cleanup, bare Bun pass/fail cleanup, stale recovery, active roots, linked trees, scan bounds, and Windows removal retries.

Closes #4762

Verification

  • bun install --frozen-lockfile
  • bun run typecheck
  • bun test tests/ci-workflows/test-runner.test.ts -t "Windows test TEMP recovery|bare .* run removes|wrapper passes parallel execution" (7 pass, 0 fail)
  • bun run privacy:scan
  • A full bun run test was attempted, but the local execution session was interrupted after its process tree had run for about 14 minutes and did not preserve a conclusive exit status. This PR remains draft pending the authoritative full-suite result.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • All CI tests are green on my local testing.

  • I pushed my PR to the latest dev commit.

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

@coderabbitai

coderabbitai Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Sep 16, 2026
@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 75 / 80

이 PR은 #4762를 고친다. Windows에서 테스트가 사용자 TEMP 아래에 opencodex-test-* / ocx-* 디렉터리를 잔뜩 남기고, 그게 쌓이면 ProfSvc 로그온이 느려진다는 실측 버그다. 현재 dev HEAD 9fc7e9b40 (패키지 2.57.0)의 scripts/test.ts를 보면 createIsolatedTestEnvironment가 mkdtempSync(join(tmpdir(), "opencodex-test-"))로 루트만 만들고, TEMP/TMP/TMPDIR은 건드리지 않는다. 자식·픽스처가 os.tmpdir()나 Windows TEMP API로 만든 트리는 그 루트 밖에 남고, tests/preload.ts의 process.on("exit") 정리도 Bun 워커에서는 자주 안 돈다. 그래서 성공한 런도 유령 디렉터리를 남긴다.

이 PR이 하는 일은 세 겹이다. (1) 새 모듈 scripts/test-temp.ts에서 런마다 루트 아래 tmp/를 만들고 TEMP/TMP/TMPDIR을 그곳으로 보낸다. (2) 루트에 .opencodex-test-owner.json 소유권 마커를 찍고, 정리 실패 시 Windows EPERM/EBUSY/ENOTEMPTY만 재시도한다. (3) Windows에서만, 테스트 시작 때 TEMP 아래 오래된(기본 48시간) opencodex/ocx 이름 트리를 한 프로세스당 한 번 스캔해 회수한다. 이름 정규식·직접 부모 소속·심볼릭 링크 없음·활성 PID 없음·메타 불일치 실패 시 스킵 같은 안전장치가 있다. tests/preload.ts는 afterAll을 주 경로로 두고 exit 훅은 보조로 남긴다. 회귀 테스트가 wrapper 정리·bare pass/fail·stale 회수·링크 트리·스캔 한도·재시도를 잡는다.

방향은 이전에 #4762 이슈 리뷰에서 말한 “TEMP를 테스트 루트 아래로 리다이렉트”와 같다. types/config 분할 캠페인과도 안 겹친다. 제품 코드가 아니라 테스트 인프라만 만진다. 미리보기 배포는 계획에 없다. 다만 지금 상태는 Draft이고, 작성자도 전체 bun run test가 약 14분 뒤에 끊겨 최종 exit를 못 남겼다고 적었다. 타깃 테스트 7개는 통과했다고 하니 설계 신뢰도는 높지만, 머지 버튼은 전체 스위트·호스티드 CI가 같은 HEAD에서 초록일 때까지 누르면 안 된다.

회수 로직의 보수성은 좋다. 잘못된 owner JSON·링크 트리·아직 살아있는 PID·48시간 미만 활동은 전부 스킵한다. 반대로 말하면, 이미 쌓인 수천 개 유령 디렉터리는 새 코드가 배포된 뒤에도 최대 48시간(+스캔 한도에 걸리면 여러 런) 동안 남을 수 있다. 그건 의도된 trade-off다. 한 번에 TEMP 전체를 비우면 위험하니까. 메인테이너가 지금 당장 워크스테이션을 비우고 싶으면, 수동 quarantine(이미 이슈에 적힌 방법)과 이 PR을 같이 쓰면 된다.

작은 주의점 두 가지. 첫째, processIsAlive는 process.kill(pid, 0)에 기대는데 Windows에서 PID 재사용이 있으면 “아직 살아 있다”로 잘못 스킵할 수 있다. 48시간 유예와 mtime 검사가 완충하지만, 완벽한 소유권 증명은 아니다. 둘째, LEGACY_OCX_TEST_ROOT 정규식은 꽤 넓어서 우연히 같은 모양의 다른 도구 디렉터리까지 후보에 넣을 수 있다. 다만 실제 삭제는 링크 없음·나이·(있으면) owner 검증을 더 지나므로 실수 삭제는 어렵다.

scripts/test-temp.ts (신규) - Windows TEMP 누수(#4762)의 본체. contain + owner 마커 + stale 회수. 현재 dev에는 이 파일이 없다.
scripts/test.ts · createIsolatedTestEnvironment - dev는 TEMP를 안 돌린다. 이 PR이 containedTemp와 recoverOnce를 넣는다. 방향 맞음.
tests/preload.ts - process.on("exit")만으로는 Bun 워커에서 정리가 빠진다. afterAll 추가는 핵심 수정.
Draft / Verification - 타깃 7개 통과, 전체 스위트 미확정. 호스티드 CI(hygiene/label/resolve-pr)도 아직 pending.
이슈 연결 - Closes #4762. 이 PR이 머지되면 #4762는 닫혀야 한다. 다른 열린 중복 수정 PR은 이 번호(#4785)가 유일하다.

메인테이너의 판단이 필요한 지점

  • Draft를 언제 Ready로 올릴지: 전체 스위트(또는 Windows 러너 CI) 한 번 초록이 최소 조건인가.
  • 48시간 유예가 너무 긴가: ProfSvc를 지금 아픈 머신에서는 수동 청소 + 짧은 유예가 나을 수도 있다.
  • 테스트 시작마다 Windows TEMP 스캔(최대 30s/1만 후보)이 CI 체감에 얼마나 닿는지.
  • PID 생존 검사만으로 owner를 믿을지, runId/락 토큰까지 더 묶을지.

너의 추천
KEEP 방향, 지금은 WAIT. Draft 유지한 채 Windows 포함 전체 테스트(또는 호스티드 CI)가 이 브랜치 HEAD에서 초록이면 Ready로 올리고 #4762와 함께 머지한다. types/config 분할과 무관하니 리베이스 캠페인에 넣지 말고, 중복 PR이 더 생기면 이 PR을 정본으로 두고 나머지를 CLOSE한다. 이미 TEMP가 수천 개인 머신은 머지 전에 이슈에 적힌 quarantine으로 한 번 비운다.

이 댓글은 grok-bot이 작성했습니다

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 76 / 80

이 PR은 Windows에서 테스트가 끝난 뒤에도 TEMP/TMP/TMPDIR 아래에 opencodex-test-*(그리고 예전 ocx-*) 폴더가 수천 개 쌓이는 #4762를 실제로 막는 코드입니다. 지금 dev HEAD는 9fc7e9b40이고, 방금 #4764(조직 단위 쿼터 거절 중 계정 로테이션 중단) → 그 아래 #4765(프롬프트 캐시 가이드를 identity-domain 문서에 인용) → #4777(업로드 파일이 있는 대화의 계정 이동 거절) 스택이 올라온 상태입니다. 그 축은 #4546 라우팅/쿼터·계정 안전이고, 이 PR은 그와 거의 겹치지 않는 테스트 하네스 위생입니다. 제품 런타임(src/codex, src/server/responses)을 건드리지 않고 scripts/ + tests/만 바꿉니다.

지금 dev의 scripts/test.ts createIsolatedTestEnvironment는 mkdtempSync(join(tmpdir(), "opencodex-test-"))로 루트만 만들고, HOME/USERPROFILE/OPENCODEX_HOME/CODEX_HOME은 그 안으로 넣지만 TEMP 계열은 그대로 호스트 TEMP를 씁니다. 그래서 테스트 안에서 또 mkdtemp를 하면 호스트 TEMP에 자식이 생기고, Windows에서 삭제가 EPERM/EBUSY로 실패하면 폴더가 남습니다. tests/preload.ts도 지금은 process.on("exit", … rmSync)만 있어서, Bun 워커가 exit 훅을 안정적으로 안 타는 경우 루트가 남습니다. #4762 리뷰(71/80)에서 “TEMP를 테스트 루트 아래로 몰고, Windows는 재시도 삭제 + 오래된 루트 회수”라고 적어 둔 방향과 이 PR이 정확히 맞습니다.

구체적으로는 새 파일 scripts/test-temp.ts가 (1) 루트에 .opencodex-test-owner.json 소유 마커를 쓰고, (2) removeTestTempTree로 EPERM/EBUSY/ENOTEMPTY만 짧게 재시도하며 지우고, (3) Windows에서만 48시간보다 오래된·이름 패턴이 맞고·심볼릭 링크 없고·소유 PID가 죽은 루트를 recoverStaleTestTempArtifacts로 지웁니다. createIsolatedTestEnvironment는 시작 시 recoverStaleTestTempArtifactsOnce를 한 번 돌리고, TEMP/TMP/TMPDIR을 join(root, "tmp")로 고정한 뒤 cleanup에서 removeTestTempTree를 씁니다. tests/preload.ts는 afterAll + exit 훅 둘 다로 같은 cleanup을 호출합니다. tests/ci-workflows/test-runner.test.ts에 stale/live/링크/스캔 한도/재시도·bare pass·fail 정리 회귀가 붙어 있고, 라벨은 bug, Closes #4762입니다. Draft이고 작성자 말에 로컬 full suite는 약 14분 뒤 세션이 끊겨 최종 exit를 못 봤다고 하니, 권위 있는 통과 증거는 아직 hosted CI/완전한 bun run test 쪽에 있습니다.

라인 쪽 문제는 “틀렸다”보다 “머지 전에 확인할 구멍”에 가깝습니다.

scripts/test-temp.ts recoverStaleTestTempArtifacts - 플랫폼이 win32가 아니면 바로 빈 결과를 돌려서, Linux/mac CI나 개발자 머신에는 오래된 opencodex-test-*가 남아도 자동 회수가 없습니다. 의도가 Windows 누수(#4762) 한정인 건 맞지만, 크로스플랫폼로 “같은 TEMP 오염”이 생기면 이 경로만으로는 안 지워집니다.

scripts/test-temp.ts processIsAlive / process.kill(pid, 0) - 소유 PID가 살아 있으면 스킵합니다. Windows에서 시그널 0 의미가 플랫폼마다 미묘하고, PID 재사용(짧게 죽은 뒤 다른 프로세스가 같은 PID) 창이 있으면 “아직 살아 있다”고 잘못 스킵하거나 그 반대가 될 수 있습니다. 48시간 grace와 이름/마커/링크 검사가 완충이지만, 실패 모드는 fail-closed(안 지움) 쪽이 안전합니다.

scripts/test-temp.ts automaticRecoveryAttempted 모듈 전역 - 한 프로세스에서 recoverStaleTestTempArtifactsOnce는 한 번만 돕니다. 테스트는 recoverStaleTestTempArtifacts를 직접 호출해서 괜찮지만, 같은 Bun 워커가 preload + wrapper를 이상하게 겹치면 “한 번만”이 예상과 다를 수 있습니다. 문서/주석으로 Once의 범위를 밝혀 두면 이후 기여자가 덜 헷갈립니다.

tests/preload.ts afterAll(cleanupIsolatedRoot) - Bun이 preload 모듈의 afterAll을 워커/파일 단위로 언제 호출하는지에 의존합니다. exit 훅을 남겨 둔 건 좋습니다. 다만 bare bun test 경로에서 afterAll이 안 도는 환경이 있으면 다시 48시간 회수에만 기대게 됩니다. 회귀 테스트의 bare pass/fail이 그 구멍을 막는 핵심이니 CI에서 그 케이스가 초록인지 꼭 봐야 합니다.

PR 상태 Draft + 로컬 full suite 미확정 - hygiene/label은 통과했고 resolve-pr는 pending이었습니다. 제품 코드가 아니라서 #4546 tip과 충돌할 가능성은 낮지만, “Windows TEMP 디스크/로그온 지연”을 고친다고 자신 있게 닫으려면 full suite(또는 최소한 Windows runner) 증거가 필요합니다.

메인테이너의 판단이 필요한 지점

  • Draft를 언제 Ready로 올릴지: 작성자가 말한 full suite 권위 결과를 이 PR CI에 맡길지, Windows self-hosted/별도 job을 요구할지.
  • #4762를 이 PR merge와 동시에 close할지, TEMP 회수가 Windows-only인 점을 이슈에 남기고 close할지.
  • 48시간 grace·최대 후보 1만·트리 25만 엔트리·30초 한도가 운영 머신(아주 더러운 TEMP)에서 충분한지, 아니면 첫 회수 후 로그 메트릭을 보고 조정할지.
  • PID alive 검사를 Windows에서 더 보수적으로(예: 마커 mtime만) 갈지, 현재 fail-closed 조합을 수용할지.

너의 추천
KEEP하되 지금은 Draft라 Ready 전환 + Windows/전체 테스트 권위 결과가 초록일 때 merge. #4762 방향과 일치하고 dev 9fc7e9b(#4764/#4765/#4777)과 겹침이 거의 없으니 rebase 전쟁 대상도 아닙니다. merge 후 #4762는 close. 로컬 full suite만 끊긴 채 Ready로 올리면 WAIT.

이 댓글은 grok-bot이 작성했습니다

@github-actions

github-actions Bot commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (0/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 0/4).

Review readiness checklist

  • ⬜ All CI tests are green on my local testing.
  • ⬜ I pushed my PR to the latest dev commit.
  • ⬜ I resolved all correct Codex and CodeRabbit findings.
  • ⬜ My PR is ready for review.

0/4 boxes ticked.

This PR stays in draft until every box above is ticked.

Co-authored-by: Valerio Coltre <colthreepv@gmail.com>
@lidge-jun

Copy link
Copy Markdown
Owner

Carried in #4796 on the current dev head, with your commit and a Co-authored-by trailer preserved. Two changes were needed on top: reclamation now requires ownership evidence, because an absent marker previously fell through to removal and every directory users have accumulated today was written by a version that stamped nothing; and the #4789 removal retry budget is folded into the same change. Cancelling the run on this head to free the queue for #4796.

lidge-jun added a commit that referenced this pull request Sep 16, 2026
…#4796)

Maintainer integration for the 2.57.0 stabilization scope. The exact head has a green aggregate ci check with no failing job. Carries #4785 and folds #4789. The containment half closes the leak in #4762 for future runs. The reclamation half needed correcting before it could ship: as authored, an absent ownership marker fell through to removal, and every directory users have accumulated today was written by a version that stamped nothing, so the rule would have deleted TEMP trees the tool cannot show it created. Reclamation now treats a missing marker as disqualifying and requires the owning pid to be dead. An already-affected workstation is not cleaned by this change; those roots are scanned, skipped and left to the user. Host-owned merge decision; no local suite, typecheck, build, or install was run.
@lidge-jun

Copy link
Copy Markdown
Owner

Landed through #4796 with your commit and attribution preserved, plus the ownership-evidence correction and the #4789 retry budget folded in. Thank you — the containment half is the actual fix for #4762.

@lidge-jun lidge-jun closed this Sep 16, 2026
agentHits pushed a commit to agentHits/opencodex that referenced this pull request Sep 17, 2026
lidge-jun#4789) (lidge-jun#4796)

Maintainer integration for the 2.57.0 stabilization scope. The exact head has a green aggregate ci check with no failing job. Carries lidge-jun#4785 and folds lidge-jun#4789. The containment half closes the leak in lidge-jun#4762 for future runs. The reclamation half needed correcting before it could ship: as authored, an absent ownership marker fell through to removal, and every directory users have accumulated today was written by a version that stamped nothing, so the rule would have deleted TEMP trees the tool cannot show it created. Reclamation now treats a missing marker as disqualifying and requires the owning pid to be dead. An already-affected workstation is not cleaned by this change; those roots are scanned, skipped and left to the user. Host-owned merge decision; no local suite, typecheck, build, or install was run.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants