feat: return interrupted status from /interrupt endpoint when prompt_id is specified#14386
feat: return interrupted status from /interrupt endpoint when prompt_id is specified#14386bollerdominik wants to merge 1 commit into
interrupted status from /interrupt endpoint when prompt_id is specified#14386Conversation
📝 WalkthroughWalkthroughThe 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
server.py (3)
1005-1005: ⚡ Quick winConsider using
get_current_queue_volatile()for consistency.Other read-only queue inspection endpoints in this file use
get_current_queue_volatile()(lines 847, 885, 922). Since this handler only reads queue state to check if a prompt_id is currently running, using the volatile version would align with the established pattern.♻️ Suggested change
- currently_running, _ = self.prompt_queue.get_current_queue() + currently_running, _ = self.prompt_queue.get_current_queue_volatile()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server.py` at line 1005, Replace the read-only call to self.prompt_queue.get_current_queue() with the volatile read variant self.prompt_queue.get_current_queue_volatile() so this handler uses the same non-mutating inspection path as other endpoints; keep the existing tuple unpacking (currently_running, _) and behavior but call get_current_queue_volatile() instead of get_current_queue().
1018-1025: ⚡ Quick winUse
web.json_response()instead of manual JSON serialization.The codebase consistently uses
web.json_response()for JSON responses throughout this file (lines 328, 334, 342, 356, 446, 697, 701, 705, etc.). Manually settingcontent_type="application/json"and usingtext=json.dumps(...)breaks this pattern and is less idiomatic for aiohttp.♻️ Suggested change
- return web.Response( - status=200, - content_type="application/json", - text=json.dumps({ - "interrupted": should_interrupt, - "prompt_id": prompt_id, - }) - ) + return web.json_response({ + "interrupted": should_interrupt, + "prompt_id": prompt_id, + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server.py` around lines 1018 - 1025, Replace the manual JSON response using web.Response with aiohttp's web.json_response to match project conventions: return web.json_response({"interrupted": should_interrupt, "prompt_id": prompt_id}, status=200). Update the code that currently constructs web.Response(...) so it uses web.json_response and preserves the status and JSON payload (references: web.Response, web.json_response, should_interrupt, prompt_id).
1029-1035: ⚡ Quick winUse
web.json_response()for consistency.Same issue as the targeted interrupt branch: manually constructing the JSON response instead of using the
web.json_response()helper breaks the established pattern in this codebase.♻️ Suggested change
- return web.Response( - status=200, - content_type="application/json", - text=json.dumps({ - "interrupted": True, - }) - ) + return web.json_response({ + "interrupted": True, + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server.py` around lines 1029 - 1035, Replace the manual JSON construction using web.Response(... text=json.dumps(...)) with aiohttp's helper: return web.json_response({"interrupted": True}, status=200) so it matches the codebase pattern; locate the return that currently creates web.Response with json.dumps and swap it to web.json_response (remove explicit content_type and text usage).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server.py`:
- Around line 998-1001: The current try/except around request.json() only
catches json.JSONDecodeError and silently sets json_data = {}, which masks other
aiohttp errors; update the handler so that either you remove the try/except to
let exceptions from request.json() propagate (matching the other POST handlers)
or catch aiohttp.web.HTTPException (and other relevant aiohttp errors) and
return an explicit HTTP error response; specifically modify the request.json()
call and the json_data assignment (replace the json.JSONDecodeError except) to
either let errors bubble up or catch aiohttp.web.HTTPException / Exception and
respond with a clear HTTPBadRequest (including an error message) instead of
defaulting to {}.
---
Nitpick comments:
In `@server.py`:
- Line 1005: Replace the read-only call to self.prompt_queue.get_current_queue()
with the volatile read variant self.prompt_queue.get_current_queue_volatile() so
this handler uses the same non-mutating inspection path as other endpoints; keep
the existing tuple unpacking (currently_running, _) and behavior but call
get_current_queue_volatile() instead of get_current_queue().
- Around line 1018-1025: Replace the manual JSON response using web.Response
with aiohttp's web.json_response to match project conventions: return
web.json_response({"interrupted": should_interrupt, "prompt_id": prompt_id},
status=200). Update the code that currently constructs web.Response(...) so it
uses web.json_response and preserves the status and JSON payload (references:
web.Response, web.json_response, should_interrupt, prompt_id).
- Around line 1029-1035: Replace the manual JSON construction using
web.Response(... text=json.dumps(...)) with aiohttp's helper: return
web.json_response({"interrupted": True}, status=200) so it matches the codebase
pattern; locate the return that currently creates web.Response with json.dumps
and swap it to web.json_response (remove explicit content_type and text usage).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| try: | ||
| json_data = await request.json() | ||
| except json.JSONDecodeError: | ||
| json_data = {} |
There was a problem hiding this comment.
Consider catching broader exceptions from request.json().
The try/except block only catches json.JSONDecodeError, but request.json() can raise other exceptions (e.g., aiohttp.web.HTTPBadRequest for invalid content-type, or other aiohttp client errors). Other POST handlers in this file (lines 930, 984, 1039, 1050) don't use try/except and let these exceptions propagate naturally, which may return proper HTTP error responses.
The current implementation silently treats any JSON parse failure as a global interrupt request (empty dict → no prompt_id). Consider whether this is the intended behavior or if you should:
- Let exceptions propagate like other handlers, or
- Catch a broader exception type and return an explicit error response
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server.py` around lines 998 - 1001, The current try/except around
request.json() only catches json.JSONDecodeError and silently sets json_data =
{}, which masks other aiohttp errors; update the handler so that either you
remove the try/except to let exceptions from request.json() propagate (matching
the other POST handlers) or catch aiohttp.web.HTTPException (and other relevant
aiohttp errors) and return an explicit HTTP error response; specifically modify
the request.json() call and the json_data assignment (replace the
json.JSONDecodeError except) to either let errors bubble up or catch
aiohttp.web.HTTPException / Exception and respond with a clear HTTPBadRequest
(including an error message) instead of defaulting to {}.
1 participant
Problem
Callers using the
/interruptAPI with an optionalprompt_idreceived no feedback on whether their request actually triggered an interrupt. Since the endpoint silently skips interruption when the prompt is not currently running, callers had no way to distinguish a successful interrupt from a no-op making it less reliable to build on.Solution
The endpoint now returns a JSON body with an
interruptedboolean:Backwards compatibility
HTTP status remains
200in all cases. Callers that only check the status code are unaffected. The response body was previously empty, so only callers explicitly asserting an empty body would be impacted which is a unlikely pattern