Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 45
feat: add node-level timeouts to prevent stuck queued states#462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
735cd6d4303a3d86342814ee52fd4be13c99839bc6de475b82c7f3c1File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -53,7 +53,8 @@ async def errored_state(namespace_name: str, state_id: PydanticObjectId, body: E | ||||||||||
| does_unites=state.does_unites, | ||||||||||
| enqueue_after= int(time.time() * 1000) + graph_template.retry_policy.compute_delay(state.retry_count + 1), | ||||||||||
| retry_count=state.retry_count + 1, | ||||||||||
| fanout_id=state.fanout_id | ||||||||||
| fanout_id=state.fanout_id, | ||||||||||
| timeout_at=state.timeout_at | ||||||||||
Comment on lines
+56
to
+57
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Retry states should get a fresh timeout window, not inherit the original deadline. Preserving Compare with Apply this diff to give retry states a fresh timeout window: retry_state = State(
node_name=state.node_name,
namespace_name=state.namespace_name,
identifier=state.identifier,
graph_name=state.graph_name,
run_id=state.run_id,
status=StateStatusEnum.CREATED,
inputs=state.inputs,
outputs={},
error=None,
parents=state.parents,
does_unites=state.does_unites,
enqueue_after= int(time.time() * 1000) + graph_template.retry_policy.compute_delay(state.retry_count + 1),
retry_count=state.retry_count + 1,
fanout_id=state.fanout_id,
- timeout_at=state.timeout_at+ timeout_minutes=state.timeout_minutes
)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents | ||||||||||
| ) | ||||||||||
| retry_state = await retry_state.insert() | ||||||||||
| logger.info(f"Retry state {retry_state.id} created for state {state_id}", x_exosphere_request_id=x_exosphere_request_id) | ||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -38,6 +38,7 @@ | ||
| from apscheduler.schedulers.asyncio import AsyncIOScheduler | ||
| from apscheduler.triggers.cron import CronTrigger | ||
| from .tasks.trigger_cron import trigger_cron | ||
| from .tasks.check_node_timeout import check_node_timeout | ||
| # init tasks | ||
| from .tasks.init_tasks import init_tasks | ||
| @@ -83,6 +84,15 @@ async def lifespan(app: FastAPI): | ||
| max_instances=1, | ||
| id="every_minute_task" | ||
| ) | ||
| scheduler.add_job( | ||
| check_node_timeout, | ||
| CronTrigger.from_crontab("* * * * *"), | ||
| replace_existing=True, | ||
| misfire_grace_time=60, | ||
| coalesce=True, | ||
| max_instances=1, | ||
| id="check_node_timeout_task" | ||
| ) | ||
Comment on lines
+87
to
+95
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not needed with db queries | ||
| scheduler.start() | ||
| # main logic of the server | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -28,6 +28,9 @@ class State(BaseDatabaseModel): | ||
| retry_count: int = Field(default=0, description="Number of times the state has been retried") | ||
| fanout_id: str = Field(default_factory=lambda: str(uuid.uuid4()), description="Fanout ID of the state") | ||
| manual_retry_fanout_id: str = Field(default="", description="Fanout ID from a manual retry request, ensuring unique retries for unite nodes.") | ||
| queued_at: Optional[int] = Field(default=None, description="Unix time in milliseconds when state was queued") | ||
| timeout_at: Optional[int] = Field(default=None, description="Unix time in milliseconds when state times out") | ||
| timeout_minutes: Optional[int] = Field(default=None, gt=0, description="Timeout in minutes for this specific state, taken from node registration") | ||
| @before_event([Insert, Replace, Save]) | ||
| def _generate_fingerprint(self): | ||
| @@ -102,5 +105,12 @@ class Settings: | ||
| ("status", 1), | ||
| ], | ||
| name="run_id_status_index" | ||
| ), | ||
| IndexModel( | ||
| [ | ||
| ("status", 1), | ||
| ("timeout_at", 1), | ||
| ], | ||
| name="timeout_query_index" | ||
| ) | ||
agam1092005 marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| ] | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. while this model of periodic jobs will work, its unnecessary as we can write a database query to figure out timeout nodes, we probably do not need to set the status timeout just from if the status is Queued and current_time > timeout_at we can figure it. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| import time | ||
| from app.models.db.state import State | ||
| from app.models.state_status_enum import StateStatusEnum | ||
| from app.singletons.logs_manager import LogsManager | ||
| logger = LogsManager().get_logger() | ||
| async def check_node_timeout(): | ||
| try: | ||
| current_time_ms = int(time.time() * 1000) | ||
| logger.info(f"Checking for timed out nodes at {current_time_ms}") | ||
| # Use database query to find and update timed out states in one operation | ||
| result = await State.get_pymongo_collection().update_many( | ||
| { | ||
| "status": StateStatusEnum.QUEUED, | ||
| "timeout_at": {"$ne": None, "$lte": current_time_ms} | ||
| }, | ||
| { | ||
| "$set": { | ||
| "status": StateStatusEnum.TIMEDOUT, | ||
| "error": "Node execution timed out" | ||
| } | ||
| } | ||
| ) | ||
Comment on lines
+16
to
+27
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainVerify retry/recovery mechanism for TIMEDOUT states. The implementation correctly marks states as Run the following script to check if there's a retry mechanism for TIMEDOUT states: 🏁 Script executed: #!/bin/bash# Description: Search for retry/recovery logic for TIMEDOUT states# Search for TIMEDOUT status handling
rg -n "TIMEDOUT" --type=py -C 5 | rg -i "retry|recover|manual"# Search for state status transitions from TIMEDOUT
ast-grep --pattern $'StateStatusEnum.TIMEDOUT'# Check if TIMEDOUT states can be manually retried or recovered
rg -n "manual_retry|retry_state" --type=py -A 10 | rg -i "timedout"Length of output: 484 Implement retry or recovery for TIMEDOUT states. I didn’t find any logic that re-queues or retries tasks once their status is set to 🤖 Prompt for AI Agents | ||
| if result.modified_count > 0: | ||
| logger.info(f"Marked {result.modified_count} states as TIMEDOUT") | ||
| except Exception: | ||
| logger.error("Error checking node timeout", exc_info=True) | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The query seems to be correct, however how will we repick timed-out states?