Skip to content

fix: localize future task times - #9462

Open
wcqqq1214 wants to merge 2 commits into
AstrBotDevs:masterfrom
wcqqq1214:fix/9447-timezone
Open

fix: localize future task times#9462
wcqqq1214 wants to merge 2 commits into
AstrBotDevs:masterfrom
wcqqq1214:fix/9447-timezone

Conversation

@wcqqq1214

@wcqqq1214 wcqqq1214 commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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.

pytest tests/unit/test_cron_tools.py tests/unit/test_cron_manager.py -q
50 passed

ruff format --check on the changed files — passed.
ruff check on the changed files — passed.

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.txt and pyproject.toml.
    / 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到 requirements.txtpyproject.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:

  • Treat naive SQLite UTC next_run_time values as UTC and convert them to the configured display timezone for both task creation and listing.
  • Avoid returning 'None' for newly scheduled or run-once tasks by falling back to scheduler lookup or the user-supplied run_at time.
  • Handle invalid timezone configurations gracefully without breaking cron tool behavior.

Enhancements:

  • Propagate the configured timezone into active-agent job scheduling to prevent unintended use of the server local timezone.
  • Expose a public get_next_run_time method on the cron manager to retrieve live next-run times from APScheduler.

Tests:

  • Add regression tests covering naive UTC timestamps, multiple timezones, invalid timezone fallback, scheduler-based next-run lookup, and run_at fallback behavior for future task tools.

@wcqqq1214
wcqqq1214 marked this pull request as ready for review July 30, 2026 10:37
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. area:core The bug / feature is about astrbot's core, backend labels Jul 30, 2026
@dosubot

dosubot Bot commented Jul 30, 2026

Copy link
Copy Markdown

📄 Knowledge review

Dosu skipped reviewing this PR because your organization has used its 200 included credits for the month. Your usage will reset on 2026-08-01. To have Dosu review this PR before then, ask your organization admin to upgrade to a pro account.


Leave Feedback Ask Dosu about AstrBot Add Dosu to your team

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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.
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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread astrbot/core/tools/cron_tools.py
"origin": "tool",
}

tz_name = str(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, None

Then 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:core The bug / feature is about astrbot's core, backend size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]future_task list工具时区问题

1 participant