Repository navigation
test(pi): inject the listing deadline in the two 50-long-session tests instead of racing 2 s - #2402
Merged
Merged
Conversation
…s instead of racing 2 s "lists the newest 50 long sessions within the deadline and reads zero bytes on repeat" (added by #2377) reads about 200 MiB of transcripts under the listing's two-second deadline (PI_READ_DEADLINE_MS, wall clock). On a busy CI runner it took 6.6 s and failed with "Pi sessions could not be read right now" (#2391's Fast checks; passed on rerun). What it checks is functional: the newest 50 rows with their titles and first prompts, zero bytes read on repeat, chunk-sized reads, an empty queue. The same race failed "lists a 202-session Pi store with 50 long transcripts through the real API" in api.contract.test.ts with a 503 (#2395's Integration run). PiSessionReader takes an optional deadlineMs beside its read seam and gives the listing's deadline through listingDeadlineMs; piSessions uses it. createServer gets a test option, piSessionDeadlineMs, for its reader. Production keeps PI_READ_DEADLINE_MS. Both tests set the deadline past their own timeout; the shared two-second deadline stays covered by the fake-clock tests. Fail-first with every 64 KiB read delayed 1 ms (scratch): the old test failed 2/2 with the same error, the new one passed 3/3. With piSessionDeadlineMs 1 (scratch) the API test gets CI's 503, so the option is wired; the API case passed 3/3, the unit file 39/39; bun run check fast ok.
📝 Summary
Merge Risk:
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title names the main change: the two 50-long-session Pi tests now inject the listing deadline instead of racing the 2 s production deadline. It follows the test(pi) convention and fits the changes… Description check The description follows the template. It has a Change section that explains the problem, the injected deadline seam, and the affected files. It has a Validation section that lists the commands run (bu… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title names the main change: the two 50-long-session Pi tests now inject the listing deadline instead of racing the 2 s production deadline. It follows the test(pi) convention and fits the changeset, which adds a deadlineMs seam to PiSessionReader and piSessionDeadlineMs to createServer. The title is 92 characters, slightly long, but it stays specific and readable.
Full details: Description check
-
Explanation
The description follows the template. It has a Change section that explains the problem, the injected deadline seam, and the affected files. It has a Validation section that lists the commands run (bun run check fast, the scoped API test command, and the unit file runs), the fail-first and wiring proofs, and the review outcomes. It does not explicitly say whether the full check or the integration suite ran, and the template's contract-test coverage line for HTTP changes is only partly addressed. Those gaps are minor.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 4 functions across 5 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Comment @coderabbitai help to get the list of available commands.
Owner
Author
|
Merging e5d8374. Fixes the two timing flakes #2377's large-store tests brought (they hit #2392, #2395 and #2397 under load): the 50-long-session unit test and the 202-session API contract test now set the Pi listing deadline past the test's own timeout through a new optional option (PiSessionReader deadlineMs, createServer piSessionDeadlineMs, default unchanged at the 2 s PI_READ_DEADLINE_MS), since they check what is listed and read, not how fast; the shared 2 s deadline stays covered with a fake clock. Production behaviour unchanged. Diff read by the merge pass; no CodeRabbit review needed. |
devswha
added a commit
that referenced
this pull request
Oct 30, 2026
…ans hit the deadline (#2410) Found by the flake-hunt lane while it fixed the #2377 timing flakes (#2402); Dori decided to fix it in its own PR. On a slow PC, **Earlier sessions** for Pi could become an error that never cleared. ## The bug (MEDIUM, user-visible) @IdrisGit's streaming reader (#2377) reads each Pi transcript in full, so native names and first prompts show even when they sit deep in the file. A listing has one shared two-second deadline. By design, a scan that cannot finish returned a retryable error, never incomplete labels: - **On a deadline:** the API answered 503 `session_history_busy`. The client retried up to six times, then emptied the list and showed "Pi sessions could not be read right now. Try again." with a Retry button. - **Finished files:** their labels were cached, so retries usually got further. - **Partial scans:** these were discarded. One transcript whose scan alone takes more than two seconds on a slow disk never finished, so Earlier sessions showed the error on every open. ## Why "never incomplete labels" is relaxed #2377's rule kept a half-read file from showing a wrong label. What it actually cost was the whole list. This PR keeps the part that matters and drops the part that hurt: - A label from a torn or unfinished read is still never shown. - A session whose label is not known yet is listed with its session ID, the same fallback a session with no name has always had. - Its label fills in on a later open, because the scan resumes where it stopped. So the list can be incomplete in its labels, but never wrong, and never empty because one file is slow. ## Design Credit to @IdrisGit: #2377's bounded streaming reader is kept whole. Its 64 KiB chunks, checked descriptors, stat-keyed cache, EOF validation and queue limits all stay. This PR changes what a deadline does and adds resumable progress: - **Rows before labels:** a header pass establishes the newest rows (up to 50) before any label budget is spent. Completed cached labels show at once; unfinished ones use the session-ID fallback. The header pass has its own 10-second deadline and reads at most one 64 KiB chunk per candidate per open. If it expires, the rows already checked are kept. Only when no header at all could be checked does it still return the retryable 503, rather than a false empty list. - **Resumable progress:** parser progress for long headers and transcripts is kept per file, keyed by its stat identity. Any change, append, truncation, rewrite or replacement discards it. A huge transcript therefore gets its label after a few opens. Headers from other folders are refused before caching, so they cannot evict that progress. - **No whole-list 503 for one file:** a label-pass error keeps the rows and withholds only that file's label. The errors are a file changing mid-scan (a running Pi agent appending), a read failure, and deletion or replacement. 503 remains only for a full queue and a stopped reader. - **Windows:** folder eligibility is checked once, in the header pass. - **Small files** are finished during the header pass, at no extra I/O. ## Proof - **Fail-first:** the original five deadline regressions failed on origin/main (0 passed, 5 failed, `PiMetadataBusy`). Each review finding was reproduced as a test before its fix: | Case | Before | After | |---|---:|---:| | Expiry before any header / older rows hidden (round 1) | failed | passed | | Long header, malformed or incomplete preamble | 3 failed | 3 passed | | Foreign-header cache churn evicting progress | 1 failed | 1 passed | | Windows identity checked twice | 1 failed | 1 passed | | Label append, read error, deletion, replacement | 4 failed | 4 passed | | Active or queued header past its deadline | 2 failed | 2 passed | | 257 headers over 64 KiB churning the cache (round 3) | 1 failed | 1 passed | - **Tests:** the Pi reader suite passed 65, the reader and history suites together 159, and the isolated history API cases 7. `bun run check fast` passed (8,810 unit tests). - **Browser, before and after:** a fictional bridge drove the real components, with one transcript whose scan exceeds an injected deadline, at desktop (1440) and phone (390) widths. - **Before (origin/main):** only "Pi sessions could not be read right now. Try again." and Retry. - **After:** every row, with the slow one shown by its ID. - **After a reopen:** every row labelled. ### Browser, before and after (one transcript's scan exceeds an injected deadline) | | Desktop (1440) | Phone (390) | |---|---|---| | Before (origin/main) | <img src="https://gist.githubusercontent.com/devswha/da985201e11483e9528eaf74e324799b/raw/b17fc96fae8baf717ddc1358b72c4cd4448a152d/pifix-before-origin-main-error-1440.png" width="420"> | <img src="https://gist.githubusercontent.com/devswha/da985201e11483e9528eaf74e324799b/raw/b17fc96fae8baf717ddc1358b72c4cd4448a152d/pifix-before-origin-main-error-390.png" width="180"> | | After: every row, the slow one shown by its ID | <img src="https://gist.githubusercontent.com/devswha/da985201e11483e9528eaf74e324799b/raw/b17fc96fae8baf717ddc1358b72c4cd4448a152d/pifix-after-all-rows-slow-unlabeled-1440.png" width="420"> | <img src="https://gist.githubusercontent.com/devswha/da985201e11483e9528eaf74e324799b/raw/b17fc96fae8baf717ddc1358b72c4cd4448a152d/pifix-after-all-rows-slow-unlabeled-390.png" width="180"> | | After a reopen: labels filled in | <img src="https://gist.githubusercontent.com/devswha/da985201e11483e9528eaf74e324799b/raw/b17fc96fae8baf717ddc1358b72c4cd4448a152d/pifix-after-reopen-labels-1440.png" width="420"> | <img src="https://gist.githubusercontent.com/devswha/da985201e11483e9528eaf74e324799b/raw/b17fc96fae8baf717ddc1358b72c4cd4448a152d/pifix-after-reopen-labels-390.png" width="180"> | Not run locally: the full Integration and native Windows lanes (CI runs them). The Windows alias case is covered deterministically. ## Reviews This PR changes server code and the history API's behaviour, so both reviewers ran, over three rounds. **omo-native-code-reviewer: APPROVE (WATCH) in round 3.** - Round 1 BLOCKed on an expiry before any header was checked returning an empty 200 (HIGH), and on rows older than the expired file being dropped (MEDIUM). Both were fixed by the header pass. - Round 2 BLOCKed on non-deadline label-pass errors (an append, a read failure, a deleted file) still turning the list into a 503. Fixed: those rows keep null or cached labels. - Round 3 confirmed every earlier finding fixed, and that the late safety correction is right: a swapped symlink or a changed identity is "no row", not an unchecked header. LOWs, kept: - If every file in a store fails to read, each open answers 503 rather than a false empty list. - An append during the one-chunk header read can hide that row for one open. - A promise-ordering race in one test is deterministic but order-sensitive. - A file whose first line is a multi-MiB malformed record takes several opens to recover. - Round 4, the delta that fixes the cache churn: APPROVE (WATCH). LOWs, kept: - The 256-entry cache is shared across folders, so three or more folders, each with 50+ headers over 64 KiB, opened in rotation could evict each other's progress. Pi does not write headers that large. - When the unfinished-header cap stops a pass, older complete sessions are missing from that one response; they fill in on later opens. **Codex (Sol 6.1 in round 1, Terra 6.1 from round 2, per the owner's switch).** - Round 1 reproduced the hidden-older-rows gap. Fixed. - Round 2 raised three HIGHs and a MEDIUM; each HIGH was reproduced as a failing test first, then fixed: - a long header causing a 503 loop; - foreign-header cache churn evicting progress; - the Windows identity being checked twice. - MEDIUM: the header pass had no time bound. It now has a 10 s deadline. - Round 3 confirmed those fixed and raised three more HIGHs: - **Cache churn across 257 files that each have a header over 64 KiB (a permanent 503).** Reproduced (0 passed, 1 failed on 98df12ca) and fixed in 606cf7b2: each header traversal now stops after 50 unfinished headers. That is independent of the 50-row limit and well below the 256-entry cache, so one traversal never evicts the newest checkpoints before the next open advances them. After the fix the test passes (1/1), the reader suite passes 61, and check fast passes (8,806 unit tests). - **A header over 64 KiB is missing from the first response.** Kept, on purpose. Real Pi headers are a few hundred bytes. Such a file is listed on a later open, and older rows show meanwhile. - **503 when no header at all could be checked** (for example `EIO` on the only file). Kept, on purpose. Without one checked header there is no trustworthy session identity to list. A 200 with no rows would claim "no earlier sessions", which is the false answer round 1 removed. The 503 makes the client retry. - Round 4, on the delta, found the same eviction class through headerless files: empty `.jsonl` files cached as `null` but not counted toward the cap. Fixed in 5405fbbc: one traversal touches at most 200 files that can take a cache entry, below the 256 slots. A 344-file repro failed before and passes after. - Its MEDIUM is kept as a known limit: 50 transcripts that are all being appended to, each with a header over 64 KiB, ahead of an older session keep that session out until they settle. Real Pi headers are a few hundred bytes. - Round 5, on 5405fbbc: both reviewers found that 200 empty `.jsonl` files newer than a real session made every listing return an empty array. Fixed in 90da040d: completed headerless results are not cached and do not count. The cap is derived from the exported `PI_CACHE_ENTRIES` (3/4 = 192). A cutoff with no rows gives the retryable 503, never an empty 200. Repros: 200 and 201 empty files, 0/2 before, 2/2 after. Cache hits still count, because counting only insertions would let a hit promote an old entry past new checkpoints; omo agreed. - Round 6, on 90da040d: omo APPROVE. Terra's last MEDIUM, the same case as omo's L13: a headerless file over 64 KiB restarted from byte 0 on every open, delaying older sessions. Fixed in 69415701: a completed empty result is cached only for files larger than one chunk, keyed by stat identity, and counts like any cached entry. Terra's 150-file repro failed before (0/1) and passes after (1/1); later unchanged opens read zero bytes. **Follow-up, outside this PR:** Terra noted that origin/main's existing 400-check discovery bound can still produce an empty list, if enough newer empty files fill the budget before a real session is discovered. That limit predates this PR. - Round 7, on 69415701: omo APPROVE (WATCH). Both reviewers named the same boundary, Terra as a MEDIUM and omo as L14: **192 or more files over 64 KiB with no session header, newer than every real session in one folder, keep the list on a retryable 503.** This is shipped as a documented limit, the same kind as the cache and discovery bounds. - Any fixed per-open budget can be filled by enough junk files. Each review round moved this threshold further out, and none removed it. - Pi writes no such files. - origin/main's existing 400-check discovery bound has the same shape. - The list never shows a false "no sessions" from this PR's paths; a cutoff with no rows answers the retryable 503. ## Known limits (documented in docs/development.md) - Labels for files beyond the per-open budgets fill in over later opens. - 192 or more headerless files over 64 KiB ahead of every real session keep a retryable 503, as above. - 50 transcripts that are all being appended to, each with a header over 64 KiB, delay older sessions until they settle. - Several folders, each with many oversized headers, opened in rotation can evict each other's progress (the cache is shared).
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
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.
Found by the flake-hunt lane, with a Dori pin. The Pi listing tests added by #2377 raced the two-second production deadline on busy runners:
server/pi-session-metadata.test.ts"lists the newest 50 long sessions within the deadline and reads zero bytes on repeat":server/api.contract.test.ts"lists a 202-session Pi store with 50 long transcripts through the real API without timing out": test(keybar): prove the shell took the bar's Enter by a command's output, not a prompt count #2395's Integration run (job 123744721790), a 503 for the same reason.Cause (test flake)
Both tests list about 200 MiB of Pi transcripts and check what is listed:
They did this under the listing's real deadline,
PI_READ_DEADLINE_MS(2 s of wall clock, queue time included). On a loaded runner, reading 200 MiB takes longer than that.Change
The deadline is injected rather than raced:
PiSessionReadertakes an optionaldeadlineMsbeside its existingreadtest seam and exposes it aslistingDeadlineMs.piSessionsuses that for the listing's deadline. Unset, it is stillPI_READ_DEADLINE_MS.createServergets a test option,piSessionDeadlineMs, passed to its reader. It is next to the other*Mstest options, such asattachHoldMs.docs/development.mdand the changelog fragment say this.The shared two-second deadline is still covered, with a fake clock: aggregate expiry, independent deadlines, queued expiry and FIFO admission, in
pi-session-metadata.test.ts:484-505,:585and:627. Those tests also assert that the default isPI_READ_DEADLINE_MS. The API's 503 shape is still checked by its own case.Proof
piSessionDeadlineMs: 1(scratch), the API case fails with CI's 503, so the option reaches the listing.bun run check run bun test ./server/api.contract.test.ts -t lists.a.202-session). The unit file passed 5/5, then 39/39 again after the wording edits.bun run check fastok.Reviews
The PR changes product code (the reader and server test seams, with no production behaviour change), so both reviewers ran.
server/index.ts:738,server/remote-entry.ts:88, the default reader, and the QA script.