resolve some issues with the task scheduler#532
resolve some issues with the task scheduler#532Giordano10 wants to merge 0 commit intosipeed:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses critical issues with the cron scheduler where tasks were triggered at incorrect times, repeated unexpectedly, and the list command exposed all users' jobs instead of filtering by chat context. The changes introduce timezone support for cron expressions and implement proper job filtering based on channel and chat ID.
Changes:
- Added timezone parameter support for cron expressions, allowing users to specify IANA timezone names
- Implemented job list filtering by channel and chat ID to prevent exposing other users' scheduled tasks
- Added test coverage for both the filtering logic and timezone-aware schedule computation
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/tools/cron_tool_test.go | New test file validating that job listing correctly filters by chat context |
| pkg/tools/cron.go | Added timezone parameter to tool schema, timezone validation, and filtering logic in listJobs() |
| pkg/cron/service_test.go | New test verifying timezone-aware next run computation for cron expressions |
| pkg/cron/service.go | Modified computeNextRun() to respect timezone for cron expressions |
| README.pt-br.md | Added documentation about cron scheduling via Telegram and fixed "Preferências" spelling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| t.Fatalf("filtered list leaked other chat jobs: %s", res.ForLLM) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The test only covers the case where context is set and filtering works correctly. Consider adding a test case for when SetContext is not called or called with empty values to verify the behavior when session context is absent. This would document whether listing all jobs without context is intentional or should return an error.
| func TestCronToolListWithoutContextListsAllJobs(t *testing.T) { | |
| storePath := t.TempDir() + "/jobs.json" | |
| cs := cron.NewCronService(storePath, nil) | |
| schedule := cron.CronSchedule{Kind: "every", EveryMS: int64Ptr(60000)} | |
| if _, err := cs.AddJob("mine", schedule, "hello", true, "telegram", "chat-a"); err != nil { | |
| t.Fatalf("failed to add job: %v", err) | |
| } | |
| if _, err := cs.AddJob("other", schedule, "hi", true, "telegram", "chat-b"); err != nil { | |
| t.Fatalf("failed to add other job: %v", err) | |
| } | |
| cfg := &config.Config{} | |
| msgBus := bus.NewMessageBus() | |
| tool := NewCronTool(cs, mockJobExecutor{}, msgBus, t.TempDir(), false, 0, cfg) | |
| // Intentionally do not set context to verify behavior without session context. | |
| res := tool.Execute(context.Background(), map[string]interface{}{ | |
| "action": "list", | |
| }) | |
| if res.IsError { | |
| t.Fatalf("unexpected error when listing without context: %s", res.ForLLM) | |
| } | |
| if !strings.Contains(res.ForLLM, "mine") { | |
| t.Fatalf("unfiltered list missing job 'mine': %s", res.ForLLM) | |
| } | |
| if !strings.Contains(res.ForLLM, "other") { | |
| t.Fatalf("unfiltered list missing job 'other': %s", res.ForLLM) | |
| } | |
| } |
| > 1. Abra a conversa com o bot já configurado. | ||
| > 2. Envie pedidos naturais, ex.: `Me lembre em 10 minutos de reiniciar o Raspberry` ou `Todo dia às 9h, me lembre de verificar os backups`. | ||
| > 3. O bot cria o job com o seu chat automaticamente; confirme enviando `Liste os agendamentos` (ou via CLI: `picoclaw cron list`). | ||
| > 4. O horário segue o fuso configurado no sistema onde o gateway roda (ex.: timezone do seu Raspberry). |
There was a problem hiding this comment.
The documentation states that scheduling follows the timezone configured on the system where the gateway runs. However, with the new timezone parameter added in this PR, users can now specify a timezone for cron expressions. Consider updating this documentation to mention that users can optionally provide a timezone parameter for cron-based schedules, while at_seconds-based reminders will still use the server's timezone. This would help users understand when and how to use the new timezone feature.
| > 4. O horário segue o fuso configurado no sistema onde o gateway roda (ex.: timezone do seu Raspberry). | |
| > 4. Por padrão, o horário segue o fuso configurado no sistema onde o gateway roda (ex.: timezone do seu Raspberry), mas para agendamentos baseados em expressões `cron` você pode informar um parâmetro `timezone`; lembretes do tipo `at_seconds` continuam usando o fuso do servidor. |
| tz = strings.TrimSpace(tz) | ||
| if _, err := time.LoadLocation(tz); err != nil { | ||
| return ErrorResult(fmt.Sprintf("invalid timezone: %v", err)) | ||
| } |
There was a problem hiding this comment.
The timezone parameter is accepted but only respected for "cron" schedule kind (lines 261-286 in service.go). When using "at_seconds" or "every_seconds", the timezone is stored in the schedule but never used. This means that one-time reminders created with "at_seconds" (e.g., "remind me in 10 minutes") will always use the server's local time, not the user's timezone. Consider either:
- Documenting that timezone only applies to cron expressions, or
- Handling timezone for "at" schedules by computing the offset and adjusting the atMS value accordingly
| } | |
| } | |
| // Timezone is only meaningful for cron expressions; for "at" and "every" schedules, | |
| // the delay is relative and independent of timezone, so we reject timezone usage there | |
| if schedule.Kind != "cron" { | |
| return ErrorResult("timezone parameter is only supported for cron_expr schedules") | |
| } |
| t.Fatalf("next run = %s, want %s", got, want) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The test validates timezone behavior for cron expressions but doesn't cover timezone handling for "at_seconds" or "every_seconds" schedule types. Since the timezone parameter can be provided for any schedule type but is only used for cron expressions, add test cases to verify that timezone is correctly ignored (or document this limitation) for "at" and "every" schedules. This would help prevent confusion about the expected behavior when users specify timezones with at_seconds parameters.
| func TestComputeNextRunIgnoresTimezoneForEvery(t *testing.T) { | |
| storePath := filepath.Join(t.TempDir(), "cron", "jobs.json") | |
| cs := NewCronService(storePath, nil) | |
| // For non-cron schedules (e.g., "every", "at"/"*_seconds"), the TZ field | |
| // is ignored and next-run computation is based solely on the schedule. | |
| now := time.Date(2024, 1, 1, 10, 0, 0, 0, time.UTC) | |
| everyMs := int64(60_000) // every 60 seconds | |
| withTZ := CronSchedule{ | |
| Kind: "every", | |
| EveryMS: &everyMs, | |
| TZ: "America/New_York", | |
| } | |
| withoutTZ := CronSchedule{ | |
| Kind: "every", | |
| EveryMS: &everyMs, | |
| } | |
| nextWithTZ := cs.computeNextRun(&withTZ, now.UnixMilli()) | |
| if nextWithTZ == nil { | |
| t.Fatalf("expected next run time for schedule with TZ, got nil") | |
| } | |
| nextWithoutTZ := cs.computeNextRun(&withoutTZ, now.UnixMilli()) | |
| if nextWithoutTZ == nil { | |
| t.Fatalf("expected next run time for schedule without TZ, got nil") | |
| } | |
| if *nextWithTZ != *nextWithoutTZ { | |
| t.Fatalf("expected timezone to be ignored for 'every' schedules; got nextWithTZ=%d, nextWithoutTZ=%d", *nextWithTZ, *nextWithoutTZ) | |
| } | |
| } |
| jobs := t.cronService.ListJobs(false) | ||
|
|
||
| t.mu.RLock() | ||
| channel := t.channel | ||
| chatID := t.chatID | ||
| t.mu.RUnlock() | ||
|
|
||
| if channel != "" && chatID != "" { | ||
| filtered := make([]cron.CronJob, 0, len(jobs)) | ||
| for _, j := range jobs { | ||
| if j.Payload.Channel == channel && j.Payload.To == chatID { | ||
| filtered = append(filtered, j) | ||
| } | ||
| } | ||
| jobs = filtered | ||
| } |
There was a problem hiding this comment.
When listing jobs without a session context (channel and chatID are empty), the filtering is skipped and all jobs are returned. This could expose jobs from other users/chats when the list command is executed from a CLI or another context where session info isn't set. Consider whether this is the intended behavior, or if the list command should require a session context (similar to how add requires it on line 139-141). If returning all jobs in CLI context is intentional, this is acceptable but should be documented.
| jobs := t.cronService.ListJobs(false) | |
| t.mu.RLock() | |
| channel := t.channel | |
| chatID := t.chatID | |
| t.mu.RUnlock() | |
| if channel != "" && chatID != "" { | |
| filtered := make([]cron.CronJob, 0, len(jobs)) | |
| for _, j := range jobs { | |
| if j.Payload.Channel == channel && j.Payload.To == chatID { | |
| filtered = append(filtered, j) | |
| } | |
| } | |
| jobs = filtered | |
| } | |
| t.mu.RLock() | |
| channel := t.channel | |
| chatID := t.chatID | |
| t.mu.RUnlock() | |
| // Require a session context when listing jobs to avoid exposing jobs | |
| // from other users or chats when called from contexts without session info. | |
| if channel == "" || chatID == "" { | |
| return SilentResult("Listing scheduled jobs requires an active chat session") | |
| } | |
| jobs := t.cronService.ListJobs(false) | |
| filtered := make([]cron.CronJob, 0, len(jobs)) | |
| for _, j := range jobs { | |
| if j.Payload.Channel == channel && j.Payload.To == chatID { | |
| filtered = append(filtered, j) | |
| } | |
| } | |
| jobs = filtered |
|
Hi there, Can you please write the PR description in English so the community is at a better place to understand your contribution? Also would love to have the doc update translated to other languages as well, even if the translation is done via AI. |
|
Oh, excuse me, I will translate the description of this PR for the community. 📝 Description 🗣️ Type of Change 📚 Technical Context (Skip for Docs) 🧪 Test Environment My code/docs follow the style of this project. I have performed a self-review of my own changes. I have updated the documentation accordingly. Regarding the README file documentation for other languages, I can do this in English and French, but I will have difficulties doing it for Eastern languages because I don't have the knowledge base to check if the translation is correct with what I described. I hope you understand. |
xiaket
changed the title
|
Thanks @Giordano10 , do you mind also updating the message in the issue as well? |
|
updated @xiaket thanks |
📝 Description
The goal of this PR is to resolve some issues with the task scheduler. When requesting picoclaw to schedule reminders at specific times, cron saves the task, but for some reason it is being triggered at different times than scheduled, and sometimes repeats the same schedule without being requested.
🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
<(#528) (Fixes #528 ) -->
📚 Technical Context (Skip for Docs)
🧪 Test Environment
📸 Evidence (Optional)
Click to view Logs/Screenshots
☑️ Checklist