Repository navigation
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Image read contract and worker server/claude-image-file.ts, server/claude-image-worker.ts, server/claude-image-reader.fake.ts |
Adds file-generation metadata and shared read state. The worker handles image reads and generation operations, then returns results to the pool. |
Bounded pool and worker lifecycle server/claude-image-pool.ts, server/claude-image-worker.fake.ts, server/claude-image-pool.test.ts |
Adds a single-worker pool with queue and waiter limits, coalescing, cancellation, deadlines, reply validation, and worker retirement. Tests cover capacity, recovery, and responsiveness. |
Authorization, generation checks, and route integration server/conversation.ts, server/pane-routes.ts, server/index.ts, server/claude-image-isolation.test.ts, changelog.d/claude-image-disk-isolation.md |
Adds deadline and abort handling, pane and transcript revalidation, and file-generation checks. The route passes its abort signal, and the server holds and releases the pool. Tests cover image results, stale reads, authorization changes, and cancellation. |
Estimated code review effort: 4 (Complex) | ~45 minutes
Change: Bug fix · Severity of issue fixed: Medium
Possibly related issues
- devswha/ovis#2602 — The issue describes synchronous Claude image scans blocking the bridge event loop; this change moves image reads into a bounded worker.
Possibly related PRs
- devswha/ovis#2604 — Adds the cooperative transcript image reader that this change moves into worker isolation.
- devswha/ovis#1935 — Adds the Claude image retrieval path that this change updates.
- devswha/ovis#1952 — Uses the conversation-image endpoint whose server-side retrieval this change updates.
Merge Risk: 
80346
-
This change moves Claude image reads onto a separate bounded worker so slow transcript disk reads no longer stall the bridge. The reviewed evidence shows no concrete regression, and the remaining upstream CI checks are the normal merge gate.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 45.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 clearly and concisely describes the main change: isolating Claude image disk reads from the bridge. Linked Issues check The PR addresses the coding requirements in #2602.server/claude-image-pool.tsprovides a separate bounded worker pool with coalescing, 8 queued jobs, 8 waiters per job, 32 total waiters, shared dea…Out of Scope Changes check The changes stay within #2602. The new pool, worker, generation helpers, conversation fencing, server lifetime hold, route signal propagation, tests, fakes, and changelog entry directly support image-…Description check The description includes the required Change and Validation sections. It explains the problem, resulting behavior, scope, linked issue, validation results, contract-test coverage, and skipped checks. … Full details: Linked Issues check
-
Explanation
The PR addresses the coding requirements in
#2602.server/claude-image-pool.tsprovides a separate bounded worker pool with coalescing, 8 queued jobs, 8 waiters per job, 32 total waiters, shared deadlines, cancellation, and retryable busy errors. The pool has no synchronous fallback and is separate from the file, command, and task pools.server/claude-image-worker.tsreceives only explicit path and authorization metadata. It performs no pane discovery, authentication, or control RPC.server/conversation.tsrevalidates pane, path, generation, and transcript state after awaited work. The implementation retains physical slots and opened descriptors until worker close. The reported focused and regression tests cover responsiveness, bytes and headers, authorization changes,/clear, generations, coalescing, limits, cleanup, recovery, and the 8 MiB boundary.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within
#2602. The new pool, worker, generation helpers, conversation fencing, server lifetime hold, route signal propagation, tests, fakes, and changelog entry directly support image-scan isolation, lifecycle safety, validation, or regression coverage. The PR explicitly limits the change to Claude image scanning and does not claim to isolate unrelated preexisting filesystem calls. No unrelated product behavior is demonstrated.
Full details: Description check
-
Explanation
The description includes the required Change and Validation sections. It explains the problem, resulting behavior, scope, linked issue, validation results, contract-test coverage, and skipped checks. It also clearly notes that upstream CI remains required.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 45.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 20 functions across 10 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate unit tests (beta)
-
- 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.
|
Independent pool and integration reviews completed before publication. Six confirmed findings were fixed:
The final integration re-review independently reran the original authorization/invalidation/generation cases: 5 passed, 0 failed. The malformed-result reproductions also passed after the fix, and the shutdown correction was independently confirmed against the actual source. No introduced material findings remain in the reviewed scope. The review covered exact owned buffer transfer, 8 MiB limits, shared opened-FD cleanup after actual close, bounded waiters/admission, cancellation/coalescing, strict physical-worker limits, pane reauthorization after awaits, and preservation of the CPU reader's exception rollback. Existing warm parsed-history caches are retained. The preexisting tail-cache assumption for an arbitrary unobserved between-request prefix rewrite followed by growth remains documented in the PR; this review does not claim to fix all such history-rewriting semantics. Full fast/native contract and performance results are listed in the PR description. |
|
Thanks @duytbp, this is careful work, and the measurement shows the problem it targets is real. We are keeping the PR open but not merging it as is. We measured the image lookup against main (which has #2604) on a 125 MB and an 18 MB Claude transcript, 6 runs per cell, with a separate client pinging the server every 10 ms during each lookup. Medians; "ping max" is the longest any other request waited:
On a network-mounted
The project's rule for performance changes is that they must be clearly better in every measured case. A future version that keeps the network-mount win would need no regression on local disks (first-image time, a transcript being appended to, a page of many images), no 503s for a normal page, and one shared transcript reader with the Claude command-history path from #2608 rather than a second process-wide worker. Thanks again for digging into this. |
|
Pushed the test-only correction Thank you for the measurements and concrete review cases. I agree that the network-mount improvement does not justify those local and normal-page regressions. I have two read-only review lanes working on reproductions and a shared-reader design with the command-history path, including append tolerance, normal-page admission, and bounded failure recovery. I will bring back the design and evidence before further implementation changes. The PR description now records the outstanding concerns. |
|
I reproduced the append and gallery regressions on
Before replacing the implementation, would this shared-reader direction fit your requirement?
There is a resource limit to agree on: Bun 1.4.2 asynchronous filesystem dispatch uses a shared native pool, and FileHandle.read exposes no dedicated-pool selector. A two-operation cap permits scheduling another path beside one stuck operation; actual progress still depends on available native-pool capacity and other users. The explicit file/command/task Worker slots remain unused, but this cannot reserve native threads or guarantee progress after a permanently stuck syscall exhausts the available native capacity. I would not describe this as hard hang isolation. Is bounded native admission acceptable, or do you require a separately isolated shared backend? The validation gate would repeat your 18/125 MB cold/warm local/network matrix with the independent 10 ms health client, including first-image and full-gallery time. It must also pass append-at-each-stage, all-24-images, blocked-path/healthy-path fairness, actual-settlement cleanup, and the existing clear/rewind/rewrite and #2608 evidence/shutdown regressions. Native async I/O is only a candidate; it still needs to demonstrate the local performance result. Coordination record: owner |
|
The reusable HTTP benchmark harness is now working with real isolated Herdr process ownership and native PID-session discovery, without snapshot/path-resolution mocks. It uses synthetic transcripts and a Node stand-in named A small local diagnostic sample (one repetition per source/size) also reproduces gallery admission failures:
Every successful body matched the exact synthetic image bytes. First/warm timing results were mixed, so I am not inferring a speedup or acceptance result from this sample. Startup is recorded separately; filesystem caches are not forcibly dropped. Runtime was Bun 1.4.2, Herdr 0.9.5, with a Node 26 stand-in. The fixture-only SSHFS lab also passed a main-only 18 MiB HTTP smoke: measured SSH RTT 14.06 ms, first image 200 in 5.60 s, health maximum 1.86 s. HTTP stays outside the delayed network namespace. This mount disables data/metadata caches and readahead, so it is a stricter synthetic setup, not your exact physical network/mount or a candidate comparison. Normal exit and SIGTERM cleanup left no lab namespace processes, mounts or isolated Herdr state. The tools and raw JSON are ready for the agreed implementation's repeated comparisons. Future baselines will include the now-merged #2608. Application code remains unchanged while the shared-reader/native-admission proposal above awaits approach agreement. |
|
Thanks @duytbp for reproducing our cases with the real route and for the HTTP benchmark harness; that is exactly the evidence this needs. Yes, the shared-reader direction fits: one in-process transcript reader (pinned async positional I/O, bounded windows, file identity, invalidation and retry), used by both the image path and the Claude command-history path from #2608, with their parsing and authority rules kept separate and no application Worker. For it to merge:
We'll review each step with the same reviewers as before. Thanks again. |
|
Thanks, I’ll follow the history-first split. The first PR will move command-history filesystem work onto the shared asynchronous reader while preserving #2608’s reconciliation and queue/rest rules. The image rewrite will follow after that foundation merges, so there is no assumed contributor-managed stack. The implementation keeps one main-thread I/O controller. Existing task workers return bounded parsed snapshots for pure reconciliation, rather than creating separate per-worker reader queues. Terminal-line proof is awaited before publishing new historical conclusions; current turn/busy processing remains independent. Recent parent/subagent tail parsing is outside this first step, and the image path is unchanged. Before pushing, I’ll check the existing cold-history/session/call/rewrite/append regressions, async-gap tracker shutdown and replacement cases, held-I/O accounting and cleanup, plus actual Windows test selection and a bundled-reader exercise. Native filesystem operations retain their physical charge until they settle; this does not claim a reserved runtime pool or forced syscall cancellation. Coordination record:
|
Change
Review status: the maintainer approved a shared asynchronous reader with a history-first split. The ready, independently main-based foundation is #2616, with full fast checks, native contracts and Linux bundle validation passed locally. This image PR is held until that foundation merges; its current worker implementation remains a superseded design and is not ready to merge. The implementation and validation below describe the existing
645b38chead, not the upcoming image rewrite.After #2604, Claude image scans yield between parser windows, but one synchronous filesystem call can still stall the bridge. Run the validated image scanner and its file-generation checks in one process-wide worker, separate from the existing file/command/task pool.
Admission is bounded to eight queued metadata jobs, eight waiters per job and 32 waiters overall. A single 15-second deadline covers authorization and queue time; client cancellation releases its waiter. Timed-out or retiring work retains the physical slot until actual close, and transcript descriptors are reclaimed only after the worker can no longer use them. The last-server shutdown unreferences the worker before termination so a blocked read does not keep the process alive.
Revalidate the pane/session/source/path after awaited work and fence the generation observed by the pinned reader. Close/clear, stale generations and canceled requests cannot serve obsolete results. Preserve the 8 MiB image limit, exact owned-buffer transfer and successful image headers. Busy/deadline responses use the existing retryable 503 policy. Warm parsed-history caches remain bounded and reusable; no decoded-image cache is added.
Fixes #2602. This is an independent main-based PR after #2604 merged.
Scope: this isolates Claude image scanning, not every preexisting filesystem call in pane discovery or other providers. It preserves the merged reader's append-only-history assumptions; the inherited tail-cache limitation for arbitrary unobserved between-request prefix rewrites followed by growth remains. Newly observed generation rejections invalidate the affected image boundary.
Validation
UNIT_TEST_WORKERS=4 bun run check fast: 10,657 passed, 0 failed, 41 skipped in 661 unit files; workflow syntax, generated types/themes, typecheck and build passed on frozen source before the latest test-only push (CI-pinned Bun 1.4.2).GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=core.excludesFile GIT_CONFIG_VALUE_0=/dev/null bun run check run bun test server/claude-session.contract.test.ts server/api.contract.test.ts: 207 passed, 0 failed, isolated Herdr. The Git override isolates the host's unrelated global.ovisignore rule.645b38cchecks original-file ownership instead; independent early-close and descriptor-leak mutations still fail as intended.