Uh oh!
There was an error while loading. Please reload this page.
feat(groomer): attribute hosted groomer aborts to pool member - #779
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
aeeedbf to
4b45c64Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
The raw fetch AbortError carried no model info, so every timeout surfaced in GroomingRun.errorMessage as "This operation was aborted" with no way to tell which pool member served it. Re-throw with the requested model and timeout duration so the 25/24h hosted aborts can be correlated with the mac member, DISPATCH_GROOMER_REPO_CONTEXT_ENABLED prompts, or specific large issues. Fixes#747 Signed-off-by: Saffron <263493777+itsmiso-ai@users.noreply.github.com>
4b45c64 to
a6ca4b7CompareThere was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: dsv4f@https://litellm.jory.dev/v1 (openai) — primary route
Recommendation
Approve. This is a small, focused change that wraps fetch AbortErrors in callGroomerLLM with the model name and timeout, directly enabling the timeout-to-pool-member correlation requested in issue PR 747. CI is green (Lint, Tests, Build, Docker Build, Typecheck, npm audit). No blockers found.
Change-by-change findings
src/lib/groomer/llm.ts
- Adds a
catchblock that detectsAbortErrorand rethrows a newErrorcontainingoptions.modelandoptions.timeoutMs. This is the core fix: the raw fetch abort carries no model info, so the wrapped message can be correlated with the pool member inGroomingRun.errorMessage. - Non-
AbortErrorerrors are rethrown unchanged, preserving existing behavior (covered by the existing "throws on fetch error" test). - The
finallyblock still clears the timeout, so no timer leak is introduced.
src/lib/groomer/llm.test.ts
- Adds a focused test that mocks
global.fetchto reject with anAbortErrorand asserts the thrown message includes the model and timeout. - The test follows the existing patterns in the file (mock
global.fetch, callcallGroomerLLM, assert on rejection).
Sources
- PR PR 779 metadata and diff
- Linked issue PR 747
- AGENTS.md repository standards
- CI check results for commit
a6ca4b7b9d841e12dbf6ce25cdf3d61ea88f5971(all success) - Repository history and impact scan
Standards Compliance
- Error handling: The new code uses the
err instanceof Errorpattern and provides a meaningful error message, matching AGENTS.md Code Standard 4. - Tests: A unit test was added for the new behavior; the repository's CI (including Tests and Typecheck) passes.
- Scope: No secrets, schema changes, API routes, Docker files, or manifests are touched. The change is confined to the groomer LLM layer and follows the existing file structure.
- No
must_checkitems were present in the PR classification.
Linked Issue Fit
Issue PR 747 reports 25 hosted groomer aborts in 24h and asks to attribute the timeouts to a pool member. This PR does exactly that at the point where the model is known: callGroomerLLM now wraps AbortError with the model name and timeout, so the error message can be correlated with the pool member.
The issue suggested run.ts/config.ts as likely files; the PR instead implements the attribution in llm.ts. That is a reasonable alternative and arguably more precise, since the model is known at the call site. The issue's "worth establishing first" correlation analysis is not performed by this PR, but the PR provides the attribution mechanism needed to perform it. The PR description says the change is in run.ts, but the diff is in llm.ts; this is only a description mismatch, not a functional problem.
Unknowns or Needs Verification
- I did not inspect
src/lib/groomer/run.tsin this review. The PR's effectiveness depends on the wrapped error message being what is persisted toGroomingRun.errorMessage; the PR's own test comment asserts this, and the linked issue's error log format is consistent with it, but it was not directly verified from the diff. - The new test simulates
AbortErrorwith a plainErrorwhosenameis set to"AbortError". I did not verify that the real runtime's fetch abort error satisfies theerr instanceof Errorcheck; if it does not, the wrapper would not trigger. This is worth a quick runtime check, but it is not a blocker based on the evidence available.
Approve: adds AbortError detection and logging in run.ts to attribute groomer aborts to pool members for timeout correlation (issue #747)
Fixes#747
Opened by foreman on review GO (workload wl-misospace-dispatch-747).