Repository navigation
fix(store): tolerate concurrent reaping in readers - #126
Conversation
063a201 to
d1fe500
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Revalidated the reader/reaper race on current main I tested a local forward-port of your reader changes, preserving today's sharded layout and Thirteen additional checks produce six failures on main and all pass with the forward-port. Controls verify that permission errors remain visible, an already-open descriptor retains its snapshot, and export's existing missing-file handling remains unchanged. HTTP reads become an empty 200 for rooms and 404 for notes. The complete local suite passes 795 tests, with one existing skip and 98.05% coverage. Ruff, formatting, ty, and caps pass; store code-lines decrease from 987 to 985. No changelog, size baseline, API shape, or dependency changes. All 138 official Chromium checks pass too. This is a local adaptation for review, not a rebase of your branch or a replacement PR. I can share the patch if useful. Tested on macOS / Python 3.12.9; Linux CI remains necessary. |
|
Thanks for rechecking this against current main, especially the retained |
|
Thanks for taking a look. Here are the patch, application instructions, and verification notes. It targets main Before sharing, I rechecked patch application and reran the tests: 6 failures / 7 passes on unchanged main; 13 passes with the fix; full suite 795 passed with one existing skip and 98.05% coverage. Ruff, format, ty and size caps pass. The gist includes reproduction steps and the patch checksum. This remains a supplemental forward-port of your fix for the existing PR, with attribution, not a replacement PR or an update to your branch. Linux checks are still outstanding; thank you for offering to run them. |
Merge main e4c4f73 into the existing PR, preserving the retained last_seq floor and original regression cases. Add lifecycle coverage for a last_seq read interrupted by reaping, and remove the old changelog edit to follow the current contribution rules. Forward-port and additional regression coverage contributed by @almondous: https://gist.github.com/almondous/2cb564dc069e17cc7dee71a9f8fc15fe
|
Thanks, @almondous. I reviewed the forward-port and reproduced the Linux regression results: 6 failures on current main, then all 13 cases passing with the fix. The retained I've incorporated your patch and tests into this PR, kept the original regressions, and added a state-machine rule for a The full Linux suite passes (799 passed, 1 skipped), along with static checks, size caps, all 138 Chromium checks, and 881 HTTP contract cases. The old changelog edit is removed to match the current contribution rules; the size baseline is unchanged. |
## What Cuts **0.14.2**. Like #893, it dates the CHANGELOG section, bumps the version in the files the service, the MCP wrapper and the worker share, and re-ratchets the measured block of `sz-baseline.json`. No cap moves. ## Why **Main CI is red.** Since #163, every push to main fails `sz.py --check` (`core/limit.py 183 -> 186`, under its cap of 193). Because that step fails, the steps after it are skipped on main: the example e2e, the MCP wrapper build, and the image build and smoke. This PR's CI runs those steps against #163 and #126 for the first time. There are three fixes since v0.14.1, all PATCH, and nothing on the HTTP surface moves: - #865: a brotli `/r/<room>/export` streams again instead of buffering to near-EOF. - #163: the per-IP token bucket's read-modify-write runs under a lock, so concurrent threadpool requests can't overspend it. - #126: room and note readers treat a file reaped between the check and `open()` as absent, where they used to return a 500. After merge: tag `v0.14.2` on the squash commit, which publishes `ghcr.io/flop-labs/technocore-chat:0.14.2`. ## Checks - [x] `uv run coverage run -m pytest tests -q && uv run coverage report`: 812 passed, 98.08% - [x] `uv run ruff check . && uv run ruff format --check .` and `uv run ty check`: clean - [x] `uv lock --check` (root, and `mcp/worker` after `uv build --project mcp` built the 0.14.2 wheel), `sz.py --caps`, `sz.py --check`, and both `release.yml` gates - [x] Docs that would now be wrong are updated: `CHANGELOG.md` is the change - [x] New surface on a world-writable service: nothing new
Two conflicts, both from main moving under this branch: - src/store.py read_messages: main (flop-labs#126) now opens the room file under suppress(FileNotFoundError) instead of checking path.exists() first, so a reader racing the reaper answers an empty room rather than a 500. Kept that, and carried this branch's head_seq tracking and clamp into it unchanged. - tests/unit/test_store.py: both sides appended tests at the end of the file; kept both, this branch's test body byte-identical.
What
Room and note readers tolerate a file being reaped just before it is opened, instead of returning an HTTP 500.
last_seqstill falls back to the retained sequence floor; permission errors still propagate.Why
Checking whether a file exists does not prevent the idle reaper from removing it before
open(). This updates the original fix for current main (e4c4f73), including its sharded storage and retained sequence state. Successful reads, exports, and write behavior are unchanged.Thanks to @almondous for the forward-port and additional regression tests. The original regressions are retained, with an additional state-machine rule for a
last_seqread interrupted by reaping. No dependencies on other open PRs.Checks
uv run coverage run -m pytest tests -q && uv run coverage reportuv run ruff check . && uv run ruff format --check .anduv run ty checkLinux regression: 6 failures before the fix, all 13 supplemental cases pass afterward. Size caps, Chromium checks, and HTTP contract checks pass. Changelog and size baselines are unchanged.