Repository navigation
fix(tui): read every age of the queue janitor before it deletes a file - #47
Merged
Merged
Conversation
The test `queue::tests::cleanup_orphans_uses_sidecar_age_for_pairs` of `pi-file-utils` was green on every earlier TUI CI run, and failed on both attempts of the run for PR #46 ("expired-by-sidecar temp removed"). The code did not change: queue.rs and queue_tests.rs are unchanged since ace8852, and PR #46 touches no Rust file (the TUI CI runs for it because it changes `pi_agent_cli/**`, which the pager's end-to-end tests drive). What changed is the runner image (`ubuntu-24.04` 20261012.334.1 on the last green runs, 20261025.353.1 on both failing attempts). Cause. A pair is two files: the temp file and `<temp>.meta.json`. The age of a pair is the `enqueued_at` in its sidecar (`pair_age`, which reads the sidecar file when it is called), and the mtime only when there is no readable sidecar. `cleanup_queue_dir` deleted files while it walked `read_dir`. When the walk met the sidecar of an expired pair first, it deleted the sidecar; the temp file then had no readable sidecar, was judged by its own mtime (fresh in the test), and stayed until a later sweep. When it met the temp file first, the temp file was removed. The order of `read_dir` is the order of the file system: by hash on ext4 (with a seed that is set when the file system is made, so a new image can list the same names in the other order), by creation on tmpfs. So the same code and the same test passed on one file system and failed on another. The comment of the function already says that pairing is decided before any deletion; the ages were not. Fix. `cleanup_queue_dir` now makes two passes: the first one decides every age (and sweeps `scratch/`, as before) and collects the expired entries; the second one deletes them. Nothing else changes: the same entries are deleted, with the same counts, and the lone-file counter still uses the name snapshot. Test. `cleanup_orphans_removes_expired_pairs_whatever_the_listing_order` makes 32 expired pairs, half of them with the temp file written first and half with the sidecar first, so that whatever order the file system lists them in, some pairs have the sidecar first. It fails on the old code on every file system that was tried, and does not depend on the order of the host. Checked: - The real crate was not built here (it needs aws-sdk and tokio and is too heavy for this machine); the `cargo nextest` job of this PR will build and run it. Instead, the old and the new janitor code (queue.rs of origin/main and of this change: `QueueItemSidecar`, `sidecar_path_for`, `cleanup_queue_dir`, `pair_age`, `is_mismatched_queue_file`, `cleanup_scratch_subdirs`, `dir_size`), and both tests, were cut out of the real files by marker (nothing retyped; the two copies differ by exactly this diff) into one small crate and run with cargo 1.94.0 (the pinned toolchain), with the build in tmpfs. - tmpfs (it lists the sidecar before the temp file): the old code fails the original test with the CI message, at the same assert; the new code passes it. - ext4 (it lists the temp file first): the old code passes the original test (which is why it passed on the old image); the new code passes it. - The new test fails on the old code on both file systems and passes on the new code on both. - rustfmt --check with tui/rustfmt.toml: the same two hunks as on origin/main (top of queue.rs), none from this change. - Runner image of the TUI CI runs: 2026-10-26 and 2026-10-27 (green): 20261012.334.1; both attempts of run 40620910485: 20261025.353.1. The order of `read_dir` on a runner cannot be seen from here, so the link between the image and the order is an inference; the reproduction above shows that the order alone gives this failure with this message. - Not touched: scripts/tui_baseline.toml and docs/baselines/tui.md. The `pi-file-utils` suite goes from 216 to 217 tests, inside the 5 % band of `reference_passed`; the floor is unchanged.
zy1233
added a commit
that referenced
this pull request
Oct 28, 2026
…ust-cache key across image updates (#48) * ci(tui): run only python_agent_e2e when only the Python agent changes A change that touches only `packages/pi-agent-cli/pi_agent_cli/**` or `packages/pi-agent-cli/tests/_tool_agent.py` started the whole TUI CI, because the end-to-end tests of the pager (`python_agent_e2e`) talk to the Python agent. The TUI CI builds and runs ten suites and takes 18 to 35 minutes. A change of the Python agent can change only that one test target. The runs of PR #46 (`--setup`, Python only) took 25 to 35 minutes, and the first two failed on a Rust test that the PR did not touch (#47). Change. - New workflow `tui-e2e.yml` (job `python_agent_e2e`): the two paths above and its own file start it. It runs `cargo nextest run --locked --no-fail-fast -p pi-pager --test python_agent_e2e`: the command that `scripts/tui_baseline.py` builds for that target of the `pi-pager` suite, with the terminal variables of `[test.env]`. The Python agent is installed as in `tui-ci.yml`, and `PI_E2E_REQUIRED=1` stays, so a missing Python is a failure. - `tui-ci.yml` no longer lists the two paths. It starts for `tui/**`, the baseline script, the manifest and its own file, and it still runs these tests. A change of both runs both workflows. - Cache: the new job restores the cache of the `test` job (same key) and never saves. A cache with the same key but fewer artifacts would stop the `test` job from saving the whole one, because a key is saved once. - Six tests in `scripts/tests/test_tui_baseline.py` keep the two workflows in line: the paths, the command, the environment, the install lines (agent, nextest) and the cache key. - AGENTS.md (root and `pi-agent-cli`), `docs/baselines/tui.md`, the comment in `scripts/tui_baseline.toml` and CHANGELOG [Unreleased] say it. Checked: - actionlint 1.7.12: no finding for the two workflows (none for the old `tui-ci.yml` either). - `scripts/tests/test_tui_baseline.py`: 56 passed, 3 skipped on Windows; 59 passed on Linux (WSL). `scripts/tests`: 230 passed, 11 skipped on Windows. Ruff check and format check pass. - 13 changes of the two workflow files (the agent path back in `tui-ci.yml`, a renamed path, a file that does not watch itself, a flag removed, another target, another TERM, `PI_HOME` set, `PI_E2E_REQUIRED=0`, another nextest version, another pip line, a cache that saves, another key, a changed event header): each one fails the test that is meant to catch it. - Not checked here: a run on GitHub. The CI of this PR runs both workflows (it changes both files). The first PR that changes only `pi_agent_cli/**` shows that the TUI CI stays away. * ci(tui): keep only the pinned Rust toolchain before rust-cache rust-cache puts the version of every Rust toolchain on the runner into its cache key. The image has its own `stable`, and an image update changes it (1.99.0 to 1.99.1 on 2026-10-25). The key changed, no cache matched, and the `test` job built everything again (35 minutes, 31 of them for the build). Add the step "Keep only the pinned Rust toolchain" after the toolchain install in every job of tui-ci.yml and in tui-e2e.yml. It makes the toolchain of tui/rust-toolchain.toml the default and uninstalls the others, so the key does not depend on the image. `add-rust-environment-hash-key: false` is not a fix: it also removes the hash of Cargo.lock, so the key never changes and the cache is never saved again. Tests (scripts/tests/test_tui_baseline.py) keep the step equal in every job and before the cache step, and keep the variables that the key hashes equal in both workflows (the end-to-end job restores the key of the `test` job). Docs: CHANGELOG.md, docs/baselines/tui.md.
zy1233
added a commit
that referenced
this pull request
Oct 30, 2026
…#55) Core, harness and cli go from 0.6.0 to 0.7.0. They pin each other with ==. The two TUI crates that zypi shows (pi-pager-bin, pi-version) and tui/Cargo.lock carry the same version (scripts/tests/test_release_versions.py). The extension packages are released with the core, so all four need pi-agent-core-lc>=0.7.0 (pi_agent_core/tests/test_extension_packaging.py), and pi-dynamic-workflows-py also needs pi-agent-harness-lc>=0.7.0. The publish step skips a version that is on PyPI already, so they get new versions: web-access, goal-x and dynamic-workflows 0.3.0 -> 0.4.0, pi-mcp 0.1.0 -> 0.2.0. The README of pi-mcp-py (its PyPI page) names the new floor. CHANGELOG.md: [Unreleased] becomes [0.7.0] - 2026-10-30. It gets an opening paragraph (the release page shows it first), entries for the site (#50 to #54), the PyPI Website link, the queue cleanup fix (#47) and the new version numbers, and the known issues of 0.6.0 that still hold. The one about F7-02 to F7-09 is gone: the fixes are in this release. The entry for the console script and the README of pi-agent-cli-lc say 0.7.0, not "the next release". Why now: the published 0.6.0 has no `--setup` and no `pi-agent-cli-lc` console script, but the site and the README on main describe both. Checks: PR checks 6 of 6, and a dry run of Release on the branch (tests, both TUI builds, Python packages): https://github.com/zy1233/pi-python/actions/runs/41037156832
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.
Summary
Fixes the red
cargo nextestjob of #46. No Rust file of #46 changed; this is an old order bug that a new runner image exposed.Why the TUI CI ran for #46:
tui-ci.ymlwatchespackages/pi-agent-cli/pi_agent_cli/**, because the pager's end-to-end tests drive the real agent. Why it failed: the runner image changed,ubuntu-24.0420261012.334.1 (the last green runs) to 20261025.353.1 (both failing attempts).fn cleanup_queue_dir(...) -> u64 { + let mut expired = Vec::new(); for entry in &entries { ... if age <= max_age { continue; } - <delete the entry> + expired.push((path, name, metadata)); } + for (path, name, metadata) in expired { <delete the entry> }Evidence
Before: run 40620910485 of feat(cli): add --setup and offer it to ACP clients as a terminal login #46, both attempts:
pi-file-utils215 passed, 1 failed,cleanup_orphans_uses_sidecar_age_for_pairs("expired-by-sidecar temp removed",queue_tests.rs:4550). Every other suite is green, including the 5,651pi-pagertests with thepython_agent_e2etests, the part that feat(cli): add --setup and offer it to ACP clients as a terminal login #46 changes.After: the CI of this PR (pending). Locally, the old and the new janitor code, cut verbatim from the files into one small crate (the real crate needs aws-sdk and was not built here), both tests copied unchanged:
New test
cleanup_orphans_removes_expired_pairs_whatever_the_listing_order: 32 expired pairs, half written temp first and half sidecar first, so some pairs have the sidecar first on any file system.rustfmt --checkwithtui/rustfmt.toml: the same two hunks as onmain(top ofqueue.rs), none from this change.The link between the image and the order on the runner is an inference: the order cannot be seen from here. The reproduction shows that the order alone gives this failure with this message.
Merge Danger
Door: two-way.
Blast Radius: the orphan sweep of the upload queue at start-up. The same entries are deleted with the same counts; only the moment of deleting moves after the last age is read. The
pi-file-utilssuite goes from 216 to 217 tests, inside the 5 % band ofreference_passed.After this merges, #46 needs a new push (for example a rebase on
main) to run its TUI CI with the fix: a re-run of the old job uses the old merge commit.