test: port upstream fixture and retry fixes (2a4b8f2) - #921
Open
ozymandiashh wants to merge 2 commits into
Open
Conversation
The CLI suite is red on this branch for two reasons that upstream already fixed and that never made it here. parser.test.ts (a)/(f): createJsonlSession stamped fixture events at a fixed 2026-05-01. Durable providers age out at 90 days, so on 2026-07-30 the fixture silently pruned to zero and both cases began failing with `expected +0 to be 200`. Confirmed directly: the identical fixture dated relative to now yields one project and 200 output tokens; with the literal it yields none. Date the events relative to now so they stay inside the window whenever the suite runs. vitest.config.ts / cache-refresh-lock: a handful of integration tests exercise real servers, spawned subprocesses and real filesystem locks, and starve under a saturated parallel run — failing closed, which is correct behaviour but not what those tests measure. A small global retry rides that out; a real regression is deterministic and fails every attempt. Two upstream hunks are deliberately not ported. cli-durable-totals already has an equivalent fix on this branch (ec449af) that clamps to a fraction of the elapsed day rather than a fixed offset. The `corrupt lock recovery` describe does not exist here, because upstream's d514459 has not been ported either. Test-only; no production code changed.
Second-reviewer findings on top of the port (f3f5814). Test-only. cache-refresh-lock-process: afterEach removed the temp roots but never killed the spawned workers, which block on their barrier files indefinitely. The success path reaps them (waitForExit after each run), but a failed assertion or waitFor timeout leaves the blocked winner behind; with the global retry: 2, every attempt then spawns a fresh pair on top of the leaked ones, so they accumulate. Verified: a forced assertion failure left 3 stray worker processes after the run before this fix, and 0 after, with the kill path running between retry attempts. afterEach now kills every child it spawned and waits for it to actually die (SIGTERM, then SIGKILL after 1s), is robust to children that already exited (exitCode !== null short-circuits), and detaches waitForExit listeners first so a SIGTERM cannot surface an unhandled rejection on top of the real failure. Upstream 2a4b8f2 has the identical leaky afterEach, so this is not a regression the port introduces; it is a latent leak the retry makes reachable. Same file: worker() resolved its fixture relative to process.cwd(), so `vitest run --root packages/cli` from the repo root spawned children at a nonexistent tests/fixtures path and every one died on ENOENT before touching a barrier. Resolve the fixture relative to this file (import.meta.dirname) so the suite works from any invocation cwd. parser.test.ts createJsonlSession: the fixture dated events at exactly now minus two days. That is safely inside the 90-day retention window but sits exactly ON the 48h 'recent' cutoff in optimize.ts (RECENT_WINDOW_MS — recent iff ts >= now-48h), so any clock skew between the helper and a consumer flips the classification, and a current-month date range (cli-date.ts `month`, which starts at local midnight of the 1st) would exclude it on the 1st-2nd. Moved the offset to 6h, clamped into the current month. Six hours keeps the events unambiguously recent (42h clear of the cutoff) and inside retention with ~89 days of headroom; the clamp keeps them inside any current-month range in any timezone (it uses the same local-calendar construction as cli-date.ts). Both existing cases that use this helper, (a) and (f), pass with the new offset.
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.
The CLI suite on this branch is red, for two reasons that
mainalready fixed and that never made it across.The aged fixture
packages/cli/tests/parser.test.ts'screateJsonlSessionstamped its events at a fixed2026-05-01. Durable providers age out at 90 days (packages/cli/src/parser.ts:1611), so on 2026-07-30 the fixture silently pruned to zero and both copilot cases began failing withexpected +0 to be 200.It looks like a decoder regression and isn't one. Driving the branch's copilot provider directly over the same fixture returns a call with
outputTokens: 200; the identical fixture dated relative to now yields one project and 200 tokens, while the2026-05-01literal yields none. The decode path is fine — the fixture simply walked out of the retention window on a fixed date.The starvation failures
A handful of integration tests exercise real servers, spawned CLI subprocesses and real filesystem locks. Under a saturated parallel run an fs op starves and the operation fails closed — correct behaviour, but not what those tests measure, and a different one trips each run. Upstream's answer is a small global retry, which this ports.
Worth knowing rather than discovering later: a global
retry: 2cannot hide a deterministic regression (the aged fixture above failed all three attempts), but it can hide a genuinely intermittent product bug, and it applies to unit tests too, where flakiness would signal real nondeterminism. Vitest still printsretry x2, so the signal stays in the logs — a test that only passes on retry is worth looking at.Two upstream hunks deliberately not ported
cli-durable-totalsalready has an equivalent fix here (ec449af), and it is the better one. Upstream clamps to "two hours ago, floored at midnight"; on any run before 02:00 that puts the first event exactly at00:00and the second exactly atnow— both on range boundaries. This branch's fraction-of-elapsed-day approach stays strictly inside(midnight, now)at any hour.{ retry: 6 }on thecorrupt lock recoverydescribe has nowhere to land: that block does not exist here, because upstream'sd514459has not been ported either. It should come across with that commit.Verification
parser.test.ts8/8 green (it failed 2/8 before, reproducibly in isolation). Full CLI suite went from 10 failures to 2; both residuals are the known starvation pair, pass 100% in isolation, and were measured on a machine under heavy parallel load.Test-only — no production code changed.