test(responses): stop the shutdown-cleanup reserve from expiring under load

[lidge-jun]

Summary

Responses previous_response_id state > shutdown cleanup failure still persists unrelated response state and reports failure is flaky on loaded runners. It failed test 2/4 on two consecutive CI runs for PR #3054, a branch that does not touch src/responses/ at all.

The test forces the shutdown drain to expire by holding an icacls gate open, then injects an unlink failure during cleanup, and asserts that an unrelated small response still replays afterwards. Drain expiry is driven by the gate, which is deterministic. The fallback reserve was not: fallbackReserveMs: 80 is a real wall-clock value measured with Date.now() in fallbackPendingResponseSpills.

On a loaded runner that 80ms window can expire before the unrelated response finishes persisting. When it does, terminalizeExhaustedShutdownFallback converts resp_cleanup_unrelated into a spill-failed tombstone, and the replay assertion fails with the raw request echoed back instead of the stored input and output.

This is a test defect, not a product defect. The drain and fallback budgets behave correctly; the test was asserting a real-time property it did not intend to assert.

The fix

Size the reserve so it cannot be the thing that runs out (totalMs: 30_120, fallbackReserveMs: 30_000). The gate still forces the drain to expire, the cleanup failure is still injected, and the unrelated response must still replay. Every assertion is unchanged.

Verification

Reproduced first, on a Linux host at origin/dev (7c68768ca), before changing anything:

• 8 parallel runs of this single test ranged from 2.08s to 109.38s wall time.
• A 6-way parallel run reproduced the failure 1 in 6.
• CI hit it on test 2/4 twice in a row on PR fix(codex): refresh expired management entitlements #3054.

After the fix, at 883faf0d9 on the same host: 10 parallel runs, 10 pass, 0 fail, with individual run times spread from 5.47s to 45.51s. The longest run is now over 500x the old reserve and still passes.

Focused suite locally: bun test tests/responses-state.test.ts — 130 pass / 0 fail / 364 assertions.

Checklist

• Tests updated for the changed behavior
• No product code changed
• Targets dev
• Docs unaffected

Summary by CodeRabbit

• Tests
  • Improved shutdown cleanup test reliability by allowing drain expiration to be controlled explicitly.
  • Added clarification ensuring unrelated response state is preserved and correctly reported as a tombstone when cleanup fails.

[github-actions]

✅ Deterministic PR hygiene checks passed.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 9ade14fd-b275-4496-8cff-f3f78eadd7cd

📥 Commits

Reviewing files that changed from the base of the PR and between 7471b26 and 883faf0.

📒 Files selected for processing (1)

• tests/responses-state.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.

---

📝 Walkthrough

Walkthrough

The shutdown cleanup failure test now uses an extended spill shutdown budget and documents that the injected ACL gate must expire the drain before the fallback reserve.

Changes

Shutdown cleanup validation

Layer / File(s)	Summary
Injected-gate shutdown budget tests/responses-state.test.ts:1220-1225	The test changes totalMs from 120 to 30_120 and fallbackReserveMs from 80 to 30_000. A comment explains that the injected ACL gate, not clock expiry, must end the drain.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to 883fa

This test-only change prevents a load-sensitive cleanup reserve from expiring before the test assertion completes, without changing product behavior. No actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: ingwannu

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: preventing the shutdown-cleanup test's fallback reserve from expiring under load. It matches the changes in tests/responses-state.test.ts and the stated pu…
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title clearly describes the main change: preventing the shutdown-cleanup test's fallback reserve from expiring under load. It matches the changes in tests/responses-state.test.ts and the stated pull request objective.

Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/spill-shutdown-test-flake

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[lidge-jun]

리뷰 · 우선순위 55 / 80

설명

이 PR은 제품 코드를 안 고친다. 고치는 것은 테스트 하나다. 이름은 shutdown cleanup failure still persists unrelated response state and reports failure다. 지금 dev HEAD는 7471b2671이다. 패키지는 2.38.0이다. 종료 때 spill 공개를 먼저 비우는 본체는 이미 #3044로 들어가 있다. 그 코드는 src/responses/state.ts의 drainResponseSpillPublications다. 제품 기본값은 전체 5초, 폴백 예약 4초다. 비우기 창은 1초다.

이 테스트가 확인하고 싶은 것은 짧다. 종료 정리가 실패해도, 그것과 상관없는 작은 응답은 남아야 하고, 나중에 다시 읽을 수 있어야 한다. 방법은 이렇다. 플랫폼을 Windows처럼 속인 뒤 icacls 문을 열어 둔다. 큰 응답을 넣어서 그 문에 걸리게 한다. 그다음 버려진 임시 파일을 지울 때 일부러 실패시킨다. 그 사이에 작은 응답 resp_cleanup_unrelated를 넣는다. flushResponseState를 부르면, 문이 열려 있으니 비우기 창은 반드시 끝난다. 그다음 폴백이 작은 응답을 저장해야 한다. 마지막에 재생하면 safe-small-input과 safe-small-output이 보여야 한다.

문제는 폴백 예약 80ms가 진짜 벽시계라는 점이다. fallbackPendingResponseSpills는 Date.now()로 마감을 잰다. 문이 비우기를 끝내는 쪽은 결정적이다. 작은 응답 저장이 80ms 안에 끝나지 않으면, terminalizeExhaustedShutdownFallback이 resp_cleanup_unrelated를 spill-failed 무덤으로 바꾼다. 재생은 저장본 대신 방금 보낸 요청만 돌려준다. 그래서 실패처럼 보인다. 제품이 틀린 게 아니다. 테스트가 의도하지 않은 실시간 성질을 검사한 것이다. #3054는 src/responses/를 안 건드렸는데도 CI test 2/4가 이 테스트로 두 번 연속 빨개졌다.

고침은 예약을 30초로 늘리는 것이다. totalMs: 30_120, fallbackReserveMs: 30_000. 비우기 창은 120ms로 남는다. 문은 그대로 비우기를 끝낸다. 정리 실패도 그대로 넣는다. 작은 응답이 다시 읽혀야 한다는 검사도 그대로다. 작성자 재현은 고치기 전 한 테스트가 2초에서 109초까지 흔들렸고, 고친 뒤 같은 호스트에서 10번 병렬이 전부 통과했다. 가장 긴 건 45초였다. 제품 파일은 안 바뀐다. types.ts/config.ts 분할과 무관하다. 중복 PR이 아니다. 프리뷰 배포는 계획에 없다. 점수 55는 ‘CI가 엉뚱한 PR을 막아서 지금 고칠 가치가 있지만, 제품 구멍은 아니다’는 뜻이다.

라인 1225 - fallbackReserveMs를 30_000으로 올렸다. CI는 bun test --timeout 60000이라 테스트 천장 60초다. 이 테스트에는 { timeout: ... }가 없다. 작성자 재현이 45초까지 갔다. 러너가 조금만 더 느리면 이번엔 검사 실패가 아니라 bun 타임아웃 flake가 된다. 옆 느린 테스트는 watchdogMs로 자기 한도를 적어 둔다.
경로 tests/responses-state.test.ts:1094 - shutdown drain cap expiry enters the synchronous spill fallback도 아직 { totalMs: 120, fallbackReserveMs: 80 }다. 이 케이스는 무관한 작은 응답을 폴백 중에 넣지 않아서 같은 실패가 안 났을 수 있다.
경로 tests/responses-state.test.ts:1163 - late async spill completion cannot overwrite the shutdown fallback도 80ms다. 큰 페이로드를 폴백에서 저장한다. 부하가 같으면 같은 시계 flake가 날 수 있다.
경로 src/responses/state.ts:575,531 - 비우기 마감과 폴백 마감이 둘 다 Date.now()다. setNowForTests는 ACL 시계고, setResponseSpillNowForTests는 spill-store 시계다. 종료 예산 시계는 따로 없다. 그래서 예약만 늘리는 고침은 종류가 맞다.
라인 1225 totalMs 30_120 - 비우기 창이 예전 40ms에서 120ms로 늘었다. 문이 잡고 있으니 비우기가 끝나는 것은 여전히 결정적이다. 테스트가 매번 약 80ms를 더 기다릴 뿐이다.

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

• 30초 예약을 이대로 둘지, 아니면 drainResponseSpillPublications의 Date.now()에 테스트 시계 훅을 달아 다음부터는 숫자를 안 불릴지
• 이웃 80ms 테스트(1094, 1163)도 이번 PR에서 같이 올릴지, 재현된 한 줄만 고칠지
• 이 테스트에 { timeout: watchdogMs(30_000) + 5_000 }를 붙여 CI 60초 천장 안에서 한도를 밝힐지

너의 추천

CI 테스트 샤드가 초록이면 dev에 머지한다. #3044 본체 위의 테스트 결함이고, 제품 코드는 그대로다. 가능하면 같은 커밋에 { timeout: ... } 한 줄을 붙인다. 이웃 80ms는 후속으로 둬도 된다. 분할 무효화·중복 닫기 해당 없음. 프리뷰 배포는 계획에 없다.

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