fix: localize future task times - #9462
Conversation
📄 Knowledge reviewDosu skipped reviewing this PR because your organization has used its |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Timezone resolution logic (tz_name/tzinfo via ZoneInfo and logging on failure) is duplicated between the create and list paths; consider extracting this into a small helper to keep behavior consistent and reduce repetition.
- The ZoneInfo lookup is wrapped in a broad
except Exception; since ZoneInfo raises a specificZoneInfoNotFoundError, narrowing the exception type would make the error handling clearer and avoid masking unrelated issues. - Accessing the timezone via
context.context.context.get_config(umo=...)in multiple places is quite nested and fragile; consider adding a helper on ContextWrapper/AstrAgentContext for retrieving the configured timezone to simplify and centralize this logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Timezone resolution logic (tz_name/tzinfo via ZoneInfo and logging on failure) is duplicated between the create and list paths; consider extracting this into a small helper to keep behavior consistent and reduce repetition.
- The ZoneInfo lookup is wrapped in a broad `except Exception`; since ZoneInfo raises a specific `ZoneInfoNotFoundError`, narrowing the exception type would make the error handling clearer and avoid masking unrelated issues.
- Accessing the timezone via `context.context.context.get_config(umo=...)` in multiple places is quite nested and fragile; consider adding a helper on ContextWrapper/AstrAgentContext for retrieving the configured timezone to simplify and centralize this logic.
## Individual Comments
### Comment 1
<location path="astrbot/core/tools/cron_tools.py" line_range="144-150" />
<code_context>
+ ).strip()
+ tzinfo = None
+ if tz_name:
+ try:
+ tzinfo = ZoneInfo(tz_name)
+ except Exception:
+ logger.warning(
+ "Invalid timezone %r in config, falling back to system timezone.",
+ tz_name,
+ )
+
try:
</code_context>
<issue_to_address>
**issue (bug_risk):** Catching a broad Exception when constructing ZoneInfo may hide unexpected errors.
This `except Exception:` will catch and hide any error during `ZoneInfo` construction, not just invalid timezone names. Please restrict it to the specific exception(s) raised for unknown/invalid timezones (e.g. `ZoneInfoNotFoundError`) so genuine bugs aren’t silently ignored while still handling bad config values.
</issue_to_address>
### Comment 2
<location path="astrbot/core/tools/cron_tools.py" line_range="136" />
<code_context>
"origin": "tool",
}
+ tz_name = str(
+ context.context.context.get_config(
+ umo=context.context.event.unified_msg_origin
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the repeated timezone resolution and datetime normalization logic into small helper functions to keep cron creation and listing paths simpler and more consistent.
You can reduce the new complexity by centralizing the repeated timezone logic into small helpers, without changing behavior.
### 1. Extract timezone-from-config helper
The `tz_name` / `tzinfo` resolution block is duplicated in `create` and `list`. A focused helper keeps it consistent and easier to change later:
```python
from typing import Tuple
def _get_user_timezone(context: ContextWrapper) -> Tuple[str | None, ZoneInfo | None]:
tz_name = str(
context.context.context.get_config(
umo=context.context.event.unified_msg_origin
).get("timezone")
or ""
).strip()
if not tz_name:
return None, None
try:
return tz_name, ZoneInfo(tz_name)
except Exception:
logger.warning(
"Invalid timezone %r in config, falling back to system timezone.",
tz_name,
)
return tz_name, None
```
Then in both branches:
```python
# create path
tz_name, tzinfo = _get_user_timezone(context)
job = await cron_mgr.add_active_job(
...,
timezone=tz_name or None,
...
)
# list path
tz_name, tzinfo = _get_user_timezone(context)
```
### 2. Extract UTC→display-timezone normalization helper
The `next_run` normalization is conceptually the same in both paths. A helper for “stored/scheduler UTC (possibly naive) → display timezone” removes duplication and inline branching:
```python
from datetime import datetime, timezone as dt_timezone
def _to_display_timezone(dt: datetime | None, tzinfo: ZoneInfo | None) -> datetime | None:
if dt is None:
return None
# DB/scheduler values are UTC, possibly naive
if dt.tzinfo is None:
dt = dt.replace(tzinfo=dt_timezone.utc)
return dt.astimezone(tzinfo) if tzinfo else dt.astimezone()
```
Use it in both places:
```python
# create path
next_run = cron_mgr.get_next_run_time(job.job_id) or job.next_run_time
if next_run is not None:
next_run = _to_display_timezone(next_run, tzinfo)
elif run_at_dt is not None:
# keep the existing "run_at is in display timezone if naive" behavior
next_run = (
run_at_dt.astimezone(tzinfo)
if run_at_dt.tzinfo
else (run_at_dt.replace(tzinfo=tzinfo) if tzinfo else run_at_dt)
)
# list path
next_run = _to_display_timezone(j.next_run_time, tzinfo)
```
This preserves all current behavior (including the run_at fallback and naive handling) but removes duplicated logic and separates concerns: config → timezone, stored UTC → display timezone.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| "origin": "tool", | ||
| } | ||
|
|
||
| tz_name = str( |
There was a problem hiding this comment.
issue (complexity): Consider extracting the repeated timezone resolution and datetime normalization logic into small helper functions to keep cron creation and listing paths simpler and more consistent.
You can reduce the new complexity by centralizing the repeated timezone logic into small helpers, without changing behavior.
1. Extract timezone-from-config helper
The tz_name / tzinfo resolution block is duplicated in create and list. A focused helper keeps it consistent and easier to change later:
from typing import Tuple
def _get_user_timezone(context: ContextWrapper) -> Tuple[str | None, ZoneInfo | None]:
tz_name = str(
context.context.context.get_config(
umo=context.context.event.unified_msg_origin
).get("timezone")
or ""
).strip()
if not tz_name:
return None, None
try:
return tz_name, ZoneInfo(tz_name)
except Exception:
logger.warning(
"Invalid timezone %r in config, falling back to system timezone.",
tz_name,
)
return tz_name, NoneThen in both branches:
# create path
tz_name, tzinfo = _get_user_timezone(context)
job = await cron_mgr.add_active_job(
...,
timezone=tz_name or None,
...
)
# list path
tz_name, tzinfo = _get_user_timezone(context)2. Extract UTC→display-timezone normalization helper
The next_run normalization is conceptually the same in both paths. A helper for “stored/scheduler UTC (possibly naive) → display timezone” removes duplication and inline branching:
from datetime import datetime, timezone as dt_timezone
def _to_display_timezone(dt: datetime | None, tzinfo: ZoneInfo | None) -> datetime | None:
if dt is None:
return None
# DB/scheduler values are UTC, possibly naive
if dt.tzinfo is None:
dt = dt.replace(tzinfo=dt_timezone.utc)
return dt.astimezone(tzinfo) if tzinfo else dt.astimezone()Use it in both places:
# create path
next_run = cron_mgr.get_next_run_time(job.job_id) or job.next_run_time
if next_run is not None:
next_run = _to_display_timezone(next_run, tzinfo)
elif run_at_dt is not None:
# keep the existing "run_at is in display timezone if naive" behavior
next_run = (
run_at_dt.astimezone(tzinfo)
if run_at_dt.tzinfo
else (run_at_dt.replace(tzinfo=tzinfo) if tzinfo else run_at_dt)
)
# list path
next_run = _to_display_timezone(j.next_run_time, tzinfo)This preserves all current behavior (including the run_at fallback and naive handling) but removes duplicated logic and separates concerns: config → timezone, stored UTC → display timezone.
Fixes #9447
Modifications / 改动点
Read the live next-run time from APScheduler through a public get_next_run_time method.
Convert naive SQLite UTC timestamps to the configured display timezone correctly.
Pass the configured timezone to active-agent scheduling.
Add regression coverage for naive UTC values, multiple timezones, scheduler lookup, and run_at fallback.
This is NOT a breaking change. / 这不是一个破坏性变更。
Screenshots or Test Results / 运行截图或测试结果
This is a backend-only change, so screenshots are not applicable.
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试,并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Localize cron future task scheduling and listing times using the configured timezone, and ensure next-run times are read reliably from the scheduler instead of potentially stale database values.
Bug Fixes:
Enhancements:
Tests: