feat: add timezone support for CRON triggers (#438) #463
Conversation
SafeDep Report Summary
Package Details
This report is generated by SafeDep Github App |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds IANA timezone support to CRON triggers across docs, SDK, state-manager models and tasks: introduces a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant SDK as Python SDK
participant API as State Manager API
participant DB as Triggers DB
rect rgb(200,230,255)
note over Client,API: Client supplies CRON triggers with optional timezone
Client->>SDK: upsert_graph({ triggers: [{ type: CRON, expression, timezone? }] })
SDK->>API: POST graph payload (includes expression + timezone|UTC)
API->>DB: Persist trigger metadata { expression, timezone, trigger_time }
end
sequenceDiagram
autonumber
participant Verifier as verify_graph / trigger_cron
participant TZ as ZoneInfo
participant Cron as croniter
participant DB as Triggers DB
rect rgb(220,255,220)
note over Verifier: Compute next run per (expression, timezone)
Verifier->>DB: Load CRON triggers
Verifier->>Verifier: Group by (expression, timezone)
loop per group
Verifier->>TZ: Resolve ZoneInfo(timezone)
Verifier->>Cron: next(datetime in timezone)
Cron-->>Verifier: next_time (tz-aware)
Verifier->>Verifier: Convert next_time -> UTC (naive)
Verifier->>DB: Upsert next trigger { expression, timezone, trigger_time(UTC) }
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
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 |
Summary of ChangesHello @spa-raj, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement by adding timezone support to CRON triggers. This allows users to define scheduled tasks that adhere to specific local time zones, automatically adjusting for Daylight Saving Time. The change impacts both the data models and the scheduling logic, ensuring that cron expressions are evaluated correctly relative to the desired timezone while maintaining UTC as the internal storage standard for trigger times. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
coderabbitai
bot
added
documentation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces timezone support for CRON triggers, a valuable feature for scheduling tasks in local timezones. The changes span documentation, the Python SDK, and the state-manager backend, and are generally well-implemented. My review has identified one critical bug concerning the use of local time instead of UTC, which could lead to incorrect trigger scheduling. I have also noted a few opportunities for performance improvements by caching timezone information. Addressing these points will enhance the robustness and efficiency of this new feature.
spa-raj
force-pushed
the
timezone-for-CRON-triggers
branch
from
94cde72 to
419c9db
Compare
Allow users to specify timezone when setting up CRON triggers to enable scheduling in local time zones with automatic DST handling. Changes: - Add timezone field to CronTrigger and DatabaseTriggers models - Implement timezone-aware cron scheduling using Python's zoneinfo - Validate timezone using IANA timezone database - Update Python SDK to support timezone parameter - Add comprehensive documentation with examples - Default to UTC for backward compatibility The timezone field is optional and defaults to "UTC". All trigger times are internally stored in UTC while croniter calculations respect the specified timezone, ensuring correct scheduling across time zones and DST transitions. Signed-off-by: Sparsh <sparsh.raj30@gmail.com>
spa-raj
force-pushed
the
timezone-for-CRON-triggers
branch
from
419c9db to
db9e0a0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
state-manager/app/models/trigger_models.py (1)
41-46: Persist the normalized cron payload back ontoTrigger.value.
CronTrigger.model_validate(self.value)returns the normalized data (e.g., timezone defaulting to"UTC"), but the result is ignored, so downstream code still seestimezone=Noneand blows up when it hitsZoneInfo. Assign the validated model back toself.valuebefore returning.if self.type == TriggerTypeEnum.CRON: - CronTrigger.model_validate(self.value) + cron = CronTrigger.model_validate(self.value) + self.value = cron.model_dump()
♻️ Duplicate comments (1)
state-manager/app/models/trigger_models.py (1)
33-34: Cache the available timezone set at module load.
available_timezones()hits the tzdata set every call; invoking it on every validation is wasteful under load. Cache it in a module-level constant and reuse for membership checks. Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
docs/docs/exosphere/triggers.md(5 hunks)python-sdk/exospherehost/models.py(1 hunks)python-sdk/exospherehost/statemanager.py(1 hunks)state-manager/app/models/db/trigger.py(1 hunks)state-manager/app/models/trigger_models.py(3 hunks)state-manager/app/tasks/trigger_cron.py(2 hunks)state-manager/app/tasks/verify_graph.py(2 hunks)state-manager/tests/unit/models/test_trigger_models.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-28T13:35:42.862Z
Learnt from: NiveditJain
PR: exospherehost/exospherehost#428
File: state-manager/app/tasks/verify_graph.py:4-5
Timestamp: 2025-09-28T13:35:42.862Z
Learning: In the cron trigger scheduling logic for state-manager/app/tasks/verify_graph.py, the system intentionally schedules at least one trigger beyond the trigger_ahead_time window to ensure continuity of scheduled executions. The current logic of appending an event then breaking is by design to guarantee "at least one next" trigger.
Applied to files:
state-manager/app/tasks/verify_graph.py
🧬 Code graph analysis (3)
state-manager/tests/unit/models/test_trigger_models.py (1)
state-manager/app/models/trigger_models.py (3)
CronTrigger(17-35)Trigger(37-47)TriggerTypeEnum(7-8)
state-manager/app/tasks/verify_graph.py (2)
state-manager/app/models/trigger_models.py (1)
TriggerTypeEnum(7-8)state-manager/app/models/db/trigger.py (1)
DatabaseTriggers(9-37)
state-manager/app/tasks/trigger_cron.py (2)
state-manager/app/models/db/trigger.py (1)
DatabaseTriggers(9-37)state-manager/app/models/trigger_models.py (1)
TriggerTypeEnum(7-8)
Add unit tests to improve code coverage for timezone support: - Tests for CronTrigger model timezone validation - Tests for create_crons function with various timezone scenarios - Tests for timezone deduplication logic - Tests for default UTC timezone behavior All tests use mocks to avoid database dependencies and rely on environment variables set by CI workflow. Signed-off-by: Sparsh <sparsh.raj30@gmail.com>
spa-raj
force-pushed
the
timezone-for-CRON-triggers
branch
from
7ed39b9 to
93071da
Compare
Add unit tests for create_next_triggers function to improve coverage: - Test timezone handling (America/New_York, UTC, Europe/London) - Test None timezone defaults to UTC - Test DuplicateKeyError handling - Test datetime object validation - Test multiple trigger creation These tests cover the new timezone-aware trigger scheduling logic. Signed-off-by: Sparsh <sparsh.raj30@gmail.com>
spa-raj
force-pushed
the
timezone-for-CRON-triggers
branch
from
2cc14e8 to
770eb4e
Compare
… reaching ZoneInfo
When clients send {"timezone": null}, the raw dictionary value is None,
which would cause ZoneInfo(None) to raise an exception. Instead, validate
each trigger.value through CronTrigger.model_validate() to normalize None
to "UTC" via the field validator.
This ensures:
- None timezone values are normalized to "UTC" before ZoneInfo creation
- No None values are persisted to the database
- Invalid timezones are caught during validation
- All timezone handling uses the validated, normalized value
Added test to verify null timezone normalization behavior.
Signed-off-by: Sparsh <sparsh.raj30@gmail.com>
Cache frequently instantiated timezone objects (UTC and available timezones)
at module level to avoid repeated overhead:
- Cache available_timezones() set in trigger_models.py to prevent repeated
filesystem queries during timezone validation
- Cache ZoneInfo("UTC") constant in trigger_cron.py and verify_graph.py
to avoid repeated ZoneInfo instantiation in hot paths
These optimizations reduce unnecessary overhead in functions that are called
frequently, particularly in cron trigger scheduling operations.
Signed-off-by: Sparsh <sparsh.raj30@gmail.com>
NiveditJain
left a comment
NiveditJain
left a comment
There was a problem hiding this comment.
also please upgrade SDK Version to: 0.0.3b2 to trigger auto publish to pypi.
https://github.com/exospherehost/exospherehost/blob/964d0f2c3487dafd82ba022f9df893a4120d9da6/python-sdk/exospherehost/_version.py#L1C1-L1C20
python-sdk/exospherehost/models.py
Outdated
| class CronTrigger(BaseModel): | ||
| expression: str = Field(..., description="Cron expression for scheduling automatic graph execution. Uses standard 5-field format: minute hour day-of-month month day-of-week. Example: '0 9 * * 1-5' for weekdays at 9 AM.") | ||
| expression: str = Field(..., description="Cron expression for scheduling automatic graph execution. Uses standard 5-field format: minute hour day-of-month month day-of-week. Example: '0 9 * * 1-5' for weekdays at 9 AM.") | ||
| timezone: str = Field(default="UTC", description="Timezone for the cron expression (e.g., 'America/New_York', 'Europe/London', 'UTC'). Defaults to 'UTC'.") |
There was a problem hiding this comment.
we should add validation for timezone in SDK also, allowing to detect failures early. We can also take this as a separate PR and issue.
There was a problem hiding this comment.
Feel free to create an issue if we are moving this to different PR.
…undant validation Changes: - Refactored Trigger.value from dict to typed TriggerValue union - Added discriminated union support using Literal type for CronTrigger.type - Removed redundant CronTrigger.model_validate() call in verify_graph.py - Updated all tests to include type field in trigger value dicts Benefits: - Single validation at request time (no re-validation needed) - Type safety with IDE autocomplete and static analysis - Cleaner code: trigger.value.expression vs trigger.value["expression"] - Extensible: easy to add new trigger types to the union - All 38 trigger-related tests passing Also bumped python-sdk version to 0.0.3b2
spa-raj
force-pushed
the
timezone-for-CRON-triggers
branch
from
2e28da4 to
421c4bf
Compare
| return v | ||
|
|
||
| # Union type for all trigger types - add new trigger types here | ||
| TriggerValue = Annotated[Union[CronTrigger], Field(discriminator="type")] |
There was a problem hiding this comment.
Lets suppose we have 2 triggers with identical values, I want to test how this approach will work.
There was a problem hiding this comment.
Added unit tests for this
| TRIGGERING = "TRIGGERING" | ||
|
|
||
| class CronTrigger(BaseModel): | ||
| type: Literal[TriggerTypeEnum.CRON] = Field(default=TriggerTypeEnum.CRON, description="Type of the trigger") |
There was a problem hiding this comment.
This tells Pydantic: "Look at the type field to determine which union variant to instantiate." Without type in CronTrigger, Pydantic won't know how to deserialize the data. I have removed type from Trigger instead and refactored the code to access it via trigger.value.type
Remove the duplicate type field from the Trigger model to eliminate redundancy with the discriminated union's type field in CronTrigger. The type is now accessed via trigger.value.type instead of trigger.type. Changes: - Remove type field from Trigger model - Update trigger.type references to trigger.value.type in verify_graph.py and upsert_graph_template.py - Update tests to use new API and add tests for duplicate trigger handling - Add docstring note about accessing type via trigger.value.type This simplifies the Trigger model API while maintaining full functionality through the discriminated union pattern. Signed-off-by: Sparsh <sparsh.raj30@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
state-manager/app/models/trigger_models.py (1)
21-21: Simplify the type field declaration.The
typefield withLiteral[TriggerTypeEnum.CRON]and a default value may be redundant when using Pydantic's discriminated unions. The discriminator onTriggerValue(line 42) already handles type-based routing. Consider removing the default and making it a required literal, or verify if the discriminator alone suffices.If the field must remain, apply this diff to make the intent clearer:
- type: Literal[TriggerTypeEnum.CRON] = Field(default=TriggerTypeEnum.CRON, description="Type of the trigger") + type: Literal[TriggerTypeEnum.CRON] = Field(..., description="Type of the trigger")This ensures callers explicitly pass the type value, aligning with the discriminated union pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
state-manager/app/controller/upsert_graph_template.py(1 hunks)state-manager/app/models/trigger_models.py(3 hunks)state-manager/app/tasks/verify_graph.py(3 hunks)state-manager/tests/unit/models/test_trigger_models.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-28T13:35:42.862Z
Learnt from: NiveditJain
PR: exospherehost/exospherehost#428
File: state-manager/app/tasks/verify_graph.py:4-5
Timestamp: 2025-09-28T13:35:42.862Z
Learning: In the cron trigger scheduling logic for state-manager/app/tasks/verify_graph.py, the system intentionally schedules at least one trigger beyond the trigger_ahead_time window to ensure continuity of scheduled executions. The current logic of appending an event then breaking is by design to guarantee "at least one next" trigger.
Applied to files:
state-manager/app/tasks/verify_graph.py
🧬 Code graph analysis (4)
state-manager/app/controller/upsert_graph_template.py (2)
state-manager/app/models/db/trigger.py (1)
DatabaseTriggers(9-37)state-manager/app/models/trigger_models.py (1)
TriggerTypeEnum(10-11)
state-manager/app/models/trigger_models.py (1)
python-sdk/exospherehost/models.py (1)
CronTrigger(162-164)
state-manager/tests/unit/models/test_trigger_models.py (1)
state-manager/app/models/trigger_models.py (3)
CronTrigger(20-39)Trigger(44-54)TriggerTypeEnum(10-11)
state-manager/app/tasks/verify_graph.py (3)
python-sdk/exospherehost/statemanager.py (1)
trigger(37-91)state-manager/app/models/trigger_models.py (1)
TriggerTypeEnum(10-11)state-manager/app/models/db/trigger.py (1)
DatabaseTriggers(9-37)
🔇 Additional comments (7)
state-manager/app/models/trigger_models.py (1)
32-39: LGTM! Timezone validation handles None correctly.The validator properly normalizes
Noneto"UTC"and validates against the cached IANA timezone database. This addresses the performance concern from past reviews by using the module-level_AVAILABLE_TIMEZONEScache.Note: If the type annotation is simplified per the previous comment, the
if v is Nonecheck can be removed as Pydantic's default handling will ensurevis neverNone.state-manager/app/controller/upsert_graph_template.py (1)
69-69: LGTM! Correctly accesses type via discriminated union.The change from
trigger.typetotrigger.value.typeproperly reflects the updatedTriggermodel structure wherevalueis a discriminated union containing theCronTriggerinstance.state-manager/tests/unit/models/test_trigger_models.py (1)
9-77: LGTM! Comprehensive timezone test coverage.The test cases thoroughly cover:
- Valid IANA timezones (America/New_York, Europe/London, Asia/Tokyo)
- Default to UTC when omitted or
None- Invalid timezone rejection
- Complex cron expressions with timezones
This provides strong validation of the timezone field behavior.
state-manager/app/tasks/verify_graph.py (4)
15-16: LGTM! Module-level UTC constant improves performance.Caching
ZoneInfo("UTC")at module level avoids repeated instantiation, addressing the performance concern from past reviews.Based on past review comments.
107-114: LGTM! Deduplication by (expression, timezone) prevents duplicate triggers.The deduplication logic correctly builds a map keyed by
(expression, timezone)pairs, ensuring that triggers with the same expression but different timezones are treated as distinct, while identical(expression, timezone)combinations are deduplicated.This addresses the past review question about handling two triggers with identical values.
116-131: LGTM! Timezone-aware scheduling with correct UTC baseline.The implementation correctly:
- Gets current time in UTC (line 116), addressing the past review concern about local time
- Converts to the trigger's timezone (line 124)
- Computes next trigger time using
croniterin that timezone (lines 125-128)- Converts back to UTC for storage (line 131)
This ensures correct DST handling and timezone-aware scheduling.
Based on past review comments.
133-143: LGTM! Timezone persisted with trigger.The
timezonefield (line 137) is correctly stored inDatabaseTriggers, enabling future trigger recreations to use the same timezone context.
…ests Change timezone field from Optional[str] to str since the default="UTC" ensures it's never None at runtime. This makes the type annotation accurately reflect the actual behavior. Changes: - Change CronTrigger.timezone from Optional[str] to str - Remove None check from timezone validator (Pydantic default handles it) - Remove Optional import (no longer needed) - Rename test_duplicate_triggers_with_identical_values to test_trigger_model_equality_with_identical_values to clarify it tests Pydantic model equality, not business logic deduplication - Remove test_none_timezone_defaults_to_utc (timezone=None no longer valid) - Add integration tests for trigger deduplication in create_crons function - test_create_crons_deduplicates_identical_triggers - test_create_crons_keeps_triggers_with_different_timezones - test_create_crons_keeps_triggers_with_different_expressions - test_create_crons_complex_deduplication_scenario The integration tests verify actual database behavior to ensure the deduplication logic in verify_graph.py correctly creates only one DatabaseTriggers row per (expression, timezone) pair. Signed-off-by: Sparsh <sparsh.raj30@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
state-manager/app/models/trigger_models.py(3 hunks)state-manager/tests/unit/models/test_trigger_models.py(1 hunks)state-manager/tests/unit/tasks/test_create_crons.py(1 hunks)state-manager/tests/unit/with_database/test_trigger_deduplication.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
state-manager/tests/unit/with_database/test_trigger_deduplication.py (5)
state-manager/app/models/db/graph_template_model.py (1)
GraphTemplate(18-347)state-manager/app/models/trigger_models.py (2)
Trigger(42-52)TriggerTypeEnum(10-11)state-manager/app/models/db/trigger.py (1)
DatabaseTriggers(9-37)state-manager/app/tasks/verify_graph.py (2)
verify_graph(148-180)create_crons(107-146)state-manager/tests/unit/with_database/conftest.py (1)
app_started(23-32)
state-manager/app/models/trigger_models.py (1)
python-sdk/exospherehost/models.py (1)
CronTrigger(162-164)
state-manager/tests/unit/tasks/test_create_crons.py (2)
state-manager/app/tasks/verify_graph.py (2)
verify_graph(148-180)create_crons(107-146)state-manager/app/models/trigger_models.py (2)
Trigger(42-52)TriggerTypeEnum(10-11)
state-manager/tests/unit/models/test_trigger_models.py (1)
state-manager/app/models/trigger_models.py (3)
CronTrigger(20-37)Trigger(42-52)TriggerTypeEnum(10-11)
🪛 GitHub Actions: State Manager Unit Tests
state-manager/tests/unit/with_database/test_trigger_deduplication.py
[error] 1-1: Pytest run failed with RuntimeError: Cannot use AsyncMongoClient in different event loop. AsyncMongoClient uses low-level asyncio APIs that bind it to the event loop it was created on. This occurs during cleanup in test_create_crons_deduplicates_identical_triggers.
[error] 1-1: Pytest run failed with RuntimeError: Cannot use AsyncMongoClient in different event loop. AsyncMongoClient uses low-level asyncio APIs that bind it to the event loop it was created on. This occurs during cleanup in test_create_crons_keeps_triggers_with_different_timezones.
[error] 1-1: Pytest run failed with RuntimeError: Cannot use AsyncMongoClient in different event loop. AsyncMongoClient uses low-level asyncio APIs that bind it to the event loop it was created on. This occurs during cleanup in test_create_crons_keeps_triggers_with_different_expressions.
[error] 1-1: Pytest run failed with RuntimeError: Cannot use AsyncMongoClient in different event loop. AsyncMongoClient uses low-level asyncio APIs that bind it to the event loop it was created on. This occurs during cleanup in test_create_crons_complex_deduplication_scenario.
🔇 Additional comments (5)
state-manager/tests/unit/models/test_trigger_models.py (2)
46-48: LGTM! Error assertions correctly handle pydantic v2 validation errors.The error assertions now use
.get("msg") or str(errors[0])to safely access the error message without assuming the presence of thectxfield, which addresses the previous validation error assertion issue.Also applies to: 58-60
116-121: LGTM! Test naming and documentation clarify scope.The test has been appropriately renamed to
test_trigger_model_equality_with_identical_valuesand includes a docstring that clarifies it verifies model-level equality only, not deduplication logic. This addresses the previous concern about test intent.state-manager/tests/unit/tasks/test_create_crons.py (1)
1-235: LGTM! Comprehensive unit test coverage with proper mocking.The test suite provides excellent coverage of timezone handling, deduplication logic, and edge cases. The use of mocks to avoid database dependencies is appropriate for unit tests.
state-manager/app/models/trigger_models.py (2)
7-8: LGTM! Timezone caching optimization applied.The module-level
_AVAILABLE_TIMEZONEScache prevents repeated filesystem queries during validation, addressing the performance concern from the previous review.Based on learnings.
23-23: LGTM! Type annotation now matches runtime behavior.The
timezonefield is correctly declared asstr(notOptional[str]), and the validator no longer includes an unnecessary None check. This aligns the type annotation with Pydantic's default handling, which ensures the field is never None when usingdefault="UTC".Also applies to: 32-37
state-manager/tests/unit/with_database/test_trigger_deduplication.py
Outdated
Show resolved
Hide resolved
…lient event loop error Add loop_scope="session" to all test decorators in test_trigger_deduplication.py to ensure tests run in the same event loop as the session-scoped app_started fixture. This fixes "Cannot use AsyncMongoClient in different event loop" errors. Signed-off-by: Sparsh <sparsh.raj30@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
state-manager/tests/unit/with_database/test_trigger_deduplication.py (1)
16-265: Event loop issue still present (see past review).The past review identified a critical issue: all four integration tests fail with
RuntimeError: Cannot use AsyncMongoClient in different event loop. The AsyncMongoClient is bound to the event loop created during app startup, but pytest-asyncio creates a separate event loop for tests.While
loop_scope="session"attempts to address this by using a session-scoped event loop, the past review indicates this is insufficient. The suggested solutions remain:
- Add a function-scoped
event_loopfixture in conftest.py- Reinitialize the database client within the
app_startedfixture per test- Configure
asyncio_mode = "auto"in pytest.iniFor reference, see the detailed solutions in the past review comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
state-manager/tests/unit/with_database/test_trigger_deduplication.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
state-manager/tests/unit/with_database/test_trigger_deduplication.py (5)
state-manager/app/models/db/graph_template_model.py (1)
GraphTemplate(18-347)state-manager/app/models/trigger_models.py (2)
Trigger(42-52)TriggerTypeEnum(10-11)state-manager/app/models/db/trigger.py (1)
DatabaseTriggers(9-37)state-manager/app/tasks/verify_graph.py (2)
verify_graph(148-180)create_crons(107-146)state-manager/tests/unit/with_database/conftest.py (1)
app_started(23-32)
🔇 Additional comments (4)
state-manager/tests/unit/with_database/test_trigger_deduplication.py (4)
17-73: Test logic and assertions are correct.The test correctly validates that three identical triggers (same expression and timezone) result in only one DatabaseTriggers row, confirming deduplication works as intended.
76-130: Test correctly validates timezone differentiation.The test appropriately verifies that triggers with identical expressions but different timezones create separate DatabaseTriggers rows, confirming that timezone is part of the deduplication key.
133-186: Test correctly validates expression differentiation.The test properly verifies that triggers with different expressions but the same timezone create separate DatabaseTriggers rows, confirming that expression is part of the deduplication key.
189-265: Complex scenario test is comprehensive.The test effectively validates a realistic scenario mixing duplicate and unique triggers, correctly expecting 4 unique (expression, timezone) pairs from 7 input triggers. The assertions verify both count and specific pairs.
state-manager/tests/unit/with_database/test_trigger_deduplication.py
Outdated
Show resolved
Hide resolved
…uplication tests Extract run_deduplication_test helper to handle common setup, teardown, and assertion logic across all 4 integration tests, reducing code from 265 to 176 lines while maintaining identical test behavior. Signed-off-by: Sparsh <sparsh.raj30@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
state-manager/app/models/db/trigger.py (1)
20-37: Unique index must include timezone to avoid cross-timezone collisions.Two CRONs with same (graph, namespace, expression) but different timezones can produce identical UTC
trigger_time, causingDuplicateKeyErrorbecausetimezoneisn’t part of the unique key.- IndexModel( - [ - ("type", 1), - ("expression", 1), - ("graph_name", 1), - ("namespace", 1), - ("trigger_time", 1), - ], - name="uniq_graph_type_expr_time", - unique=True - ), + # NOTE: rename index and include timezone in the uniqueness constraint + IndexModel( + [ + ("type", 1), + ("expression", 1), + ("timezone", 1), + ("graph_name", 1), + ("namespace", 1), + ("trigger_time", 1), + ], + name="uniq_graph_type_expr_tz_time", + unique=True + ),Migration: drop
uniq_graph_type_expr_timebefore creating the new one to avoid conflicts. I can provide a migration script on request.state-manager/app/tasks/trigger_cron.py (3)
71-80: Persist normalized timezone instead of propagating None.Prevents new rows with
timezone=Noneand simplifies querying.- timezone=trigger.timezone, + timezone=tz_name,
81-84: Improve duplicate-key log context.Include timezone and time to help triage which row collided.
- except DuplicateKeyError: - logger.error(f"Duplicate trigger found for expression {trigger.expression}") + except DuplicateKeyError: + logger.error( + f"Duplicate trigger for expr={trigger.expression} tz={tz_name} time={next_trigger_time!s}" + )
112-116: Use UTC now to match UTC-naivetrigger_timein DB.
datetime.now()is local time; comparing with UTC-naive timestamps risks skew.- cron_time = datetime.now() + # Use UTC (naive) to compare with stored UTC-naive trigger_time values + cron_time = datetime.utcnow()state-manager/app/tasks/verify_graph.py (2)
140-147: Heads-up: DB unique index must include timezone.
DatabaseTriggers.Settingsstill has uniqueness withouttimezone. Without adjusting the index (see model review), two timezones with same UTCtrigger_timewill collide duringinsert_many. Please align the index first.
151-152: Addordered=Falsetoinsert_manyand handle duplicate-key errors under concurrent verification.The risk is real:
verify_graphruns as a FastAPI background task (upsert_graph_template.py:72), and concurrent requests for the same graph will trigger simultaneousverify_graphinvocations. Both will build identicalDatabaseTriggersrecords. The secondinsert_manywill fail becauseDatabaseTriggershas a unique constraint on(type, expression, graph_name, namespace, trigger_time). Currently, the error is unhandled and crashes the task.Use
ordered=Falseto attempt all document insertions regardless of errors, then catchBulkWriteError(andDuplicateKeyErrorfor safety). This pattern is already proven increate_next_states.py:235-236.state-manager/tests/unit/tasks/test_trigger_cron.py (1)
257-277: Fix failing test: addtimezoneon spec’d mocks or let code default.This test uses
MagicMock(spec=DatabaseTriggers)without assigningtimezone, causingAttributeError. Either set it explicitly or rely on the new getattr-based default once applied.- trigger = MagicMock(spec=DatabaseTriggers) + trigger = MagicMock(spec=DatabaseTriggers) + trigger.timezone = "UTC"Alternatively, keep the test as-is if you adopt the
getattr(..., None)fix increate_next_triggers.
♻️ Duplicate comments (3)
state-manager/app/tasks/trigger_cron.py (1)
15-17: Good: cache UTC ZoneInfo at module scope.Reduces repeated instantiation and aligns with prior feedback.
state-manager/app/tasks/verify_graph.py (2)
17-19: Good: module-level UTC ZoneInfo cache.Matches the pattern used elsewhere and prior guidance.
120-120: Slight simplification for UTC-naive “now”.Optional:
datetime.utcnow()is simpler for a UTC-naive baseline.- current_time = datetime.now(UTC).replace(tzinfo=None) + current_time = datetime.utcnow()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
state-manager/app/models/db/trigger.py(1 hunks)state-manager/app/tasks/trigger_cron.py(2 hunks)state-manager/app/tasks/verify_graph.py(3 hunks)state-manager/tests/unit/tasks/test_trigger_cron.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-28T13:35:42.862Z
Learnt from: NiveditJain
PR: exospherehost/exospherehost#428
File: state-manager/app/tasks/verify_graph.py:4-5
Timestamp: 2025-09-28T13:35:42.862Z
Learning: In the cron trigger scheduling logic for state-manager/app/tasks/verify_graph.py, the system intentionally schedules at least one trigger beyond the trigger_ahead_time window to ensure continuity of scheduled executions. The current logic of appending an event then breaking is by design to guarantee "at least one next" trigger.
Applied to files:
state-manager/app/tasks/verify_graph.py
🧬 Code graph analysis (3)
state-manager/app/tasks/verify_graph.py (3)
python-sdk/exospherehost/statemanager.py (1)
trigger(37-91)state-manager/app/models/trigger_models.py (1)
TriggerTypeEnum(10-11)state-manager/app/models/db/trigger.py (1)
DatabaseTriggers(9-53)
state-manager/tests/unit/tasks/test_trigger_cron.py (1)
state-manager/app/tasks/trigger_cron.py (1)
create_next_triggers(52-88)
state-manager/app/tasks/trigger_cron.py (2)
state-manager/app/models/db/trigger.py (1)
DatabaseTriggers(9-53)state-manager/app/models/trigger_models.py (1)
TriggerTypeEnum(10-11)
🪛 GitHub Actions: State Manager Unit Tests
state-manager/tests/unit/tasks/test_trigger_cron.py
[error] 272-272: test_create_next_triggers_creates_future_trigger failed due to AttributeError: Mock object has no attribute 'timezone' when accessing trigger.timezone in create_next_triggers. Update test to ensure mocked trigger has a timezone attribute or adjust code to handle missing timezone. Command: uv run pytest tests/ --cov=app --cov-report=xml --cov-report=term-missing --cov-report=html -v --junitxml=full-pytest-report.xml
state-manager/app/tasks/trigger_cron.py
[error] 56-56: pytest failed with AttributeError: Mock object has no attribute 'timezone' during create_next_triggers; the test mocks for DatabaseTriggers without providing a timezone attribute on the trigger mock. Command: uv run pytest tests/ --cov=app --cov-report=xml --cov-report=term-missing --cov-report=html -v --junitxml=full-pytest-report.xml
🔇 Additional comments (5)
state-manager/app/tasks/trigger_cron.py (1)
62-88: Loop guarantees “at least one next” trigger — good.Insert-then-break ensures one future trigger beyond
cron_time, matching scheduling expectations.Based on learnings
state-manager/tests/unit/tasks/test_trigger_cron.py (4)
24-49: LGTM: verifies propagation for America/New_York.Good coverage for tz pass-through and expression.
50-73: LGTM: UTC path covered.Confirms tz propagation and base behavior.
334-355: LGTM: multiple future triggers case.Covers the “every 6h” scenario and minimum count.
74-97: Implementation mismatch: verify if timezone normalization is being added.The current implementation at line 74 (
timezone=trigger.timezone) passes None directly to the database without normalization. The test's current assertion (assert call_kwargs['timezone'] is None) correctly reflects this behavior.The suggested assertion change (
assert call_kwargs['timezone'] == "UTC") would only be correct if the implementation is also modified to normalize None to "UTC" before construction. Without seeing those implementation changes, the assertion would fail.Clarify:
- Will the implementation at line 74 be updated to normalize None? (e.g.,
timezone=trigger.timezone or "UTC")- Or should the test remain as-is, reflecting the current storage of None with a comment documenting that timezone normalization happens at read time?
| # Use the trigger's timezone, defaulting to UTC if not specified | ||
| tz = ZoneInfo(trigger.timezone or "UTC") | ||
|
|
||
| # Convert trigger_time to the specified timezone for croniter | ||
| trigger_time_tz = trigger.trigger_time.replace(tzinfo=UTC).astimezone(tz) | ||
| iter = croniter.croniter(trigger.expression, trigger_time_tz) | ||
|
|
There was a problem hiding this comment.
Back-compat: handle missing timezone attribute and avoid ZoneInfo(None).
Tests and older records may not have timezone. Also persist a normalized tz name for reuse later.
- # Use the trigger's timezone, defaulting to UTC if not specified
- tz = ZoneInfo(trigger.timezone or "UTC")
+ # Resolve timezone safely for old records/tests without the attribute
+ tz_name = getattr(trigger, "timezone", None) or "UTC"
+ tz = ZoneInfo(tz_name)
- # Convert trigger_time to the specified timezone for croniter
- trigger_time_tz = trigger.trigger_time.replace(tzinfo=UTC).astimezone(tz)
- iter = croniter.croniter(trigger.expression, trigger_time_tz)
+ # Convert trigger_time to the specified timezone for croniter
+ trigger_time_tz = trigger.trigger_time.replace(tzinfo=UTC).astimezone(tz)
+ cron_iter = croniter.croniter(trigger.expression, trigger_time_tz)Also avoids shadowing built-in iter.
🧰 Tools
🪛 GitHub Actions: State Manager Unit Tests
[error] 56-56: pytest failed with AttributeError: Mock object has no attribute 'timezone' during create_next_triggers; the test mocks for DatabaseTriggers without providing a timezone attribute on the trigger mock. Command: uv run pytest tests/ --cov=app --cov-report=xml --cov-report=term-missing --cov-report=html -v --junitxml=full-pytest-report.xml
🤖 Prompt for AI Agents
In state-manager/app/tasks/trigger_cron.py around lines 55–61, avoid calling
ZoneInfo(None) by reading the timezone safely (e.g. tz_name = getattr(trigger,
"timezone", None) or trigger.timezone) and defaulting tz_name to "UTC" if falsy,
then create ZoneInfo(tz_name); persist the normalized tz_name back onto the
trigger record for future reuse; compute trigger_time_tz by replacing UTC on
trigger.trigger_time and astimezone(tz) as before; and stop shadowing the
built-in iter by renaming the croniter instance variable (e.g. cron_iter).
| # Get next trigger time in the specified timezone | ||
| next_trigger_time_tz = iter.get_next(datetime) | ||
|
|
||
| # Convert back to UTC for storage | ||
| next_trigger_time = next_trigger_time_tz.astimezone(UTC).replace(tzinfo=None) | ||
| expires_at = next_trigger_time + timedelta(hours=retention_hours) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consistency: consider storing expires_at as UTC-aware to match mark_ functions.*
Minor, but keeps a single convention.
- next_trigger_time_tz = iter.get_next(datetime)
+ next_trigger_time_tz = cron_iter.get_next(datetime)
- # Convert back to UTC for storage
- next_trigger_time = next_trigger_time_tz.astimezone(UTC).replace(tzinfo=None)
- expires_at = next_trigger_time + timedelta(hours=retention_hours)
+ # Convert back to UTC for storage
+ next_trigger_time = next_trigger_time_tz.astimezone(UTC).replace(tzinfo=None)
+ expires_at = next_trigger_time.replace(tzinfo=timezone.utc) + timedelta(hours=retention_hours)| # Get current time in the specified timezone | ||
| current_time_tz = current_time.replace(tzinfo=UTC).astimezone(tz) | ||
| iter = croniter.croniter(expression, current_time_tz) | ||
|
|
||
| # Get next trigger time in the specified timezone | ||
| next_trigger_time_tz = iter.get_next(datetime) | ||
|
|
||
| next_trigger_time = iter.get_next(datetime) | ||
| # Convert back to UTC for storage (remove timezone info for storage) | ||
| next_trigger_time = next_trigger_time_tz.astimezone(UTC).replace(tzinfo=None) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Name nit + consistency for expires_at.
Avoid shadowing iter and consider UTC-aware expires_at like in mark functions.
- current_time_tz = current_time.replace(tzinfo=UTC).astimezone(tz)
- iter = croniter.croniter(expression, current_time_tz)
+ current_time_tz = current_time.replace(tzinfo=UTC).astimezone(tz)
+ cron_iter = croniter.croniter(expression, current_time_tz)
@@
- next_trigger_time_tz = iter.get_next(datetime)
+ next_trigger_time_tz = cron_iter.get_next(datetime)
@@
- next_trigger_time = next_trigger_time_tz.astimezone(UTC).replace(tzinfo=None)
- expires_at = next_trigger_time + timedelta(hours=settings.trigger_retention_hours)
+ next_trigger_time = next_trigger_time_tz.astimezone(UTC).replace(tzinfo=None)
+ expires_at = next_trigger_time.replace(tzinfo=timezone.utc) + timedelta(hours=settings.trigger_retention_hours)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In state-manager/app/tasks/verify_graph.py around lines 127 to 135, avoid
shadowing the built-in-like name "iter" by renaming it (e.g., cron_iter) and
keep the computed expiry timestamp UTC-aware to match other mark functions:
compute next_trigger_time_tz from cron_iter.get_next(datetime) then convert to
UTC but do NOT strip tzinfo (i.e., set tzinfo=UTC or use astimezone(UTC) and
keep tzinfo), and assign that UTC-aware datetime to expires_at (or
next_trigger_time) for consistent storage and comparisons.
- Introduced a field validator for the 'timezone' attribute in the CronTrigger model to ensure it contains a valid IANA timezone. - Utilized the available_timezones from the zoneinfo module to validate the input, raising a ValueError for invalid timezones. This enhancement improves the robustness of the CronTrigger configuration by preventing incorrect timezone entries.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
python-sdk/exospherehost/models.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
python-sdk/exospherehost/models.py (1)
state-manager/app/models/trigger_models.py (1)
validate_timezone(34-37)
🔇 Additional comments (2)
python-sdk/exospherehost/models.py (2)
4-6: LGTM! Efficient timezone validation setup.The module-level initialization of
_AVAILABLE_TIMEZONESis appropriate since the set of available timezones is static and this enables O(1) membership checks during validation.
165-166: LGTM! Backward-compatible timezone support.The timezone field with default "UTC" correctly maintains backward compatibility while enabling the new timezone functionality. The field descriptions are clear and helpful.
| @field_validator('timezone') | ||
| @classmethod | ||
| def validate_timezone(cls, v: str) -> str: | ||
| if v not in _AVAILABLE_TIMEZONES: | ||
| raise ValueError(f"Invalid timezone: {v}. Must be a valid IANA timezone (e.g., 'America/New_York', 'Europe/London', 'UTC')") | ||
| return v |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Strip whitespace for consistency with other validators.
All other validators in this file (validate_node_name, validate_identifier, validate_next_nodes, etc.) strip whitespace from input before validation. The timezone validator should follow this pattern to prevent confusing validation errors when users accidentally include leading/trailing spaces.
Apply this diff to maintain consistency:
@field_validator('timezone')
@classmethod
def validate_timezone(cls, v: str) -> str:
- if v not in _AVAILABLE_TIMEZONES:
+ trimmed_v = v.strip()
+ if trimmed_v not in _AVAILABLE_TIMEZONES:
raise ValueError(f"Invalid timezone: {v}. Must be a valid IANA timezone (e.g., 'America/New_York', 'Europe/London', 'UTC')")
- return v
+ return trimmed_v🤖 Prompt for AI Agents
In python-sdk/exospherehost/models.py around lines 168 to 173, the timezone
validator currently checks v against _AVAILABLE_TIMEZONES without trimming
whitespace; update it to strip leading/trailing whitespace from v before
validation and return the stripped value so inputs like " UTC " validate
consistently with other validators in the file. Ensure you call v = v.strip()
(or equivalent) at the start of the validator, use the stripped value for the
membership check, and return the stripped string.
- Moved the "type": "CRON" field to the correct location within the triggers body in the StateManager class. - Ensured proper structure for trigger configuration, enhancing clarity and functionality.
- Updated test cases in test_trigger_cron.py to use timezone-aware datetime by replacing naive datetime.now() with datetime.now(timezone.utc). - Added timezone attribute to the trigger mock to ensure consistency in testing future triggers.
|
@coderabbitai review |
|
/gemini review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request introduces timezone support for CRON triggers, a valuable feature enhancement. The implementation is thorough, with updates to the Python SDK, backend models using modern Pydantic features like discriminated unions, core scheduling logic, and comprehensive documentation. The addition of extensive unit and integration tests is particularly commendable and greatly improves confidence in the changes. I've found a couple of critical issues regarding data integrity in the database schema and a bug in the trigger update logic that should be addressed. I've also made a suggestion to improve logging for easier debugging. Overall, this is a strong contribution that will be excellent once the identified issues are resolved.
- Added filterwarnings configuration to ignore deprecation warnings from Pydantic and lazy_model, improving test output clarity and reducing noise during test runs.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
state-manager/tests/unit/tasks/test_trigger_cron.py (2)
452-453: Fragile assertion: use kwargs when checking retention_hoursThe test assumes retention_hours is a positional arg. If the implementation uses a keyword arg, this fails. Check kwargs first and fall back to args.
- for call in mock_handle.call_args_list: - assert call[0][1] == 24 # retention_hours + for c in mock_handle.call_args_list: + if c.kwargs: + assert c.kwargs.get('retention_hours') == 24 + else: + assert len(c.args) >= 2 and c.args[1] == 24
257-278: Add an assertion that next trigger is strictly in the future (and UTC-naive)This ensures loop termination semantics and storage convention are preserved.
await create_next_triggers(trigger, cron_time, 24) # Verify at least one trigger was created assert MockDatabaseTriggers.called assert mock_instance.insert.called + # Last created trigger_time should be > cron_time and UTC-naive + last_kwargs = MockDatabaseTriggers.call_args[1] + assert last_kwargs['trigger_time'] > cron_time + assert last_kwargs['trigger_time'].tzinfo is None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
state-manager/tests/unit/tasks/test_trigger_cron.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
state-manager/tests/unit/tasks/test_trigger_cron.py (2)
state-manager/app/tasks/trigger_cron.py (1)
create_next_triggers(52-88)state-manager/app/models/db/trigger.py (1)
DatabaseTriggers(9-53)
🔇 Additional comments (1)
state-manager/tests/unit/tasks/test_trigger_cron.py (1)
2-4: Docs LGTMClear description and CI note.
| @pytest.mark.asyncio | ||
| async def test_create_next_triggers_with_america_new_york_timezone(): | ||
| """Test create_next_triggers processes America/New_York timezone correctly""" | ||
| trigger = MagicMock() | ||
| trigger.expression = "0 9 * * *" | ||
| trigger.timezone = "America/New_York" | ||
| trigger.trigger_time = datetime(2025, 10, 4, 13, 0, 0) # Naive UTC time | ||
| trigger.graph_name = "test_graph" | ||
| trigger.namespace = "test_namespace" | ||
|
|
||
| cron_time = datetime(2025, 10, 6, 0, 0, 0) | ||
|
|
||
| with patch('app.tasks.trigger_cron.DatabaseTriggers') as mock_db_class: | ||
| mock_instance = MagicMock() | ||
| mock_instance.insert = AsyncMock() | ||
| mock_db_class.return_value = mock_instance | ||
|
|
||
| await create_next_triggers(trigger, cron_time, 24) | ||
|
|
||
| # Verify DatabaseTriggers was instantiated with timezone | ||
| assert mock_db_class.called | ||
| call_kwargs = mock_db_class.call_args[1] | ||
| assert call_kwargs['timezone'] == "America/New_York" | ||
| assert call_kwargs['expression'] == "0 9 * * *" | ||
|
|
||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Unique index should include timezone to avoid cross‑TZ collisions
Relevant model snippet shows unique index on (type, expression, graph_name, namespace, trigger_time) without timezone. For per‑TZ schedules, identical UTC trigger_time across different timezones can collide. Include timezone in the unique key or otherwise disambiguate. Based on relevant code snippets.
Proposed index adjustment (outside this file):
# app/models/db/trigger.py
IndexModel(
[
("type", 1),
("expression", 1),
("timezone", 1), # add timezone
("graph_name", 1),
("namespace", 1),
("trigger_time", 1),
],
name="uniq_graph_type_expr_tz_time",
unique=True
)🏁 Script executed:
#!/bin/bash
#!/usr/bin/env bash
set -euo pipefail
echo "Show unique indexes defined on DatabaseTriggers:"
rg -nPU '(?s)class\s+DatabaseTriggers\b.*?indexes\s*=\s*\[' -C5 app/models | sed -n '1,300p'Length of output: 99
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for DatabaseTriggers class definition
echo "=== Finding DatabaseTriggers class ==="
fd -type f -name "*.py" | xargs rg -l "class DatabaseTriggers" | head -5
echo -e "\n=== Searching for indexes or unique constraints ==="
fd -type f -name "*.py" | xargs rg -n "unique\s*=" -A2 -B2 | grep -i trigger | head -30Length of output: 248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Content of trigger.py ==="
cat state-manager/app/models/db/trigger.py
echo -e "\n=== Searching for 'unique' in the file ==="
rg -n "unique" state-manager/app/models/db/trigger.py -B5 -A5Length of output: 2461
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Finding create_next_triggers function ==="
rg -n "def create_next_triggers" --type py -A 20 | head -50
echo -e "\n=== Searching for trigger_time assignment/calculation ==="
rg -n "trigger_time\s*=" state-manager/app/tasks/trigger_cron.py -B3 -A3 | head -40Length of output: 2522
Model schema requires timezone in unique index to prevent cross-timezone collisions
The test is correct, but the underlying database schema has a defect. The unique index at state-manager/app/models/db/trigger.py (lines 27–37) excludes timezone despite the field existing and being used per-trigger. Since trigger_time is always normalized to UTC before storage (line 67 of trigger_cron.py), two schedules with identical expressions but different timezones could theoretically produce the same UTC trigger_time when timezone offsets align or during DST transitions, violating the uniqueness constraint.
Update the unique index in state-manager/app/models/db/trigger.py to include timezone:
IndexModel(
[
("type", 1),
("expression", 1),
("timezone", 1), # add timezone
("graph_name", 1),
("namespace", 1),
("trigger_time", 1),
],
name="uniq_graph_type_expr_tz_time",
unique=True
)🤖 Prompt for AI Agents
In state-manager/app/models/db/trigger.py around lines 27 to 37, the unique
index definition omits the timezone field; modify the IndexModel field list to
insert ("timezone", 1) among the indexed keys (e.g., after ("expression", 1))
and update the index name to reflect the change (e.g.,
"uniq_graph_type_expr_tz_time"); ensure the index is recreated/applied so the
new unique constraint including timezone is enforced.
| @pytest.mark.asyncio | ||
| async def test_create_next_triggers_with_none_timezone_defaults_to_utc(): | ||
| """Test create_next_triggers with None timezone defaults to UTC""" | ||
| trigger = MagicMock() | ||
| trigger.expression = "0 9 * * *" | ||
| trigger.timezone = None | ||
| trigger.trigger_time = datetime(2025, 10, 4, 9, 0, 0) | ||
| trigger.graph_name = "test_graph" | ||
| trigger.namespace = "test_namespace" | ||
|
|
||
| cron_time = datetime(2025, 10, 6, 0, 0, 0) | ||
|
|
||
| with patch('app.tasks.trigger_cron.DatabaseTriggers') as mock_db_class: | ||
| mock_instance = MagicMock() | ||
| mock_instance.insert = AsyncMock() | ||
| mock_db_class.return_value = mock_instance | ||
|
|
||
| await create_next_triggers(trigger, cron_time, 24) | ||
|
|
||
| # Verify None timezone is passed through (will default to UTC in ZoneInfo call) | ||
| call_kwargs = mock_db_class.call_args[1] | ||
| assert call_kwargs['timezone'] is None | ||
|
|
||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Persist default timezone as "UTC" instead of None
Passing timezone=None to DatabaseTriggers contradicts “default to UTC” and can break dedupe/by‑TZ logic and analytics. Normalize to "UTC" at write time and assert accordingly in the test.
Apply this test change:
- # Verify None timezone is passed through (will default to UTC in ZoneInfo call)
- call_kwargs = mock_db_class.call_args[1]
- assert call_kwargs['timezone'] is None
+ # Verify None timezone is normalized and persisted as "UTC"
+ call_kwargs = mock_db_class.call_args[1]
+ assert call_kwargs['timezone'] == "UTC"Also update create_next_triggers to persist a normalized timezone (outside this file):
# app/tasks/trigger_cron.py
# ...
tz = ZoneInfo(trigger.timezone or "UTC")
# ...
await DatabaseTriggers(
# ...
timezone=trigger.timezone or "UTC",
# ...
).insert()🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check timezone propagation in create_next_triggers:"
rg -nPU '(?s)def\s+create_next_triggers\([^)]*\):.*?DatabaseTriggers\((?s).*timezone\s*=' -C2 app | sed -n '1,200p'
echo
echo "Grepping timezone field default/type in DatabaseTriggers:"
rg -n 'class DatabaseTriggers|timezone:\s*' -C2 app/models | sed -n '1,200p'Length of output: 95
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Directory structure at repo root ==="
fd -t d -d 2 | head -20
echo
echo "=== Find trigger_cron.py ==="
fd trigger_cron.py
echo
echo "=== Find files with DatabaseTriggers ==="
fd -t f -e py | xargs rg -l DatabaseTriggers | head -10
echo
echo "=== Find files with create_next_triggers ==="
fd -t f -e py | xargs rg -l create_next_triggers | head -10Length of output: 1005
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Top-level directory structure ==="
ls -la | head -20
echo
echo "=== Find Python files with 'create_next_triggers' ==="
find . -type f -name "*.py" | xargs grep -l "create_next_triggers" 2>/dev/null | head -10
echo
echo "=== Find Python files with 'DatabaseTriggers' ==="
find . -type f -name "*.py" | xargs grep -l "DatabaseTriggers" 2>/dev/null | head -10
echo
echo "=== Look for trigger_cron.py anywhere ==="
find . -type f -name "*trigger_cron*" 2>/dev/nullLength of output: 2020
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== create_next_triggers function ==="
sed -n '1,300p' state-manager/app/tasks/trigger_cron.py | cat -n
echo
echo "=== DatabaseTriggers model definition ==="
cat state-manager/app/models/db/trigger.py | cat -n | head -100
echo
echo "=== Test method in question (lines 74-97) ==="
sed -n '74,97p' state-manager/tests/unit/tasks/test_trigger_cron.py | cat -nLength of output: 8887
Persist default timezone as "UTC" instead of None
The implementation at state-manager/app/tasks/trigger_cron.py line 74 passes timezone=trigger.timezone directly to DatabaseTriggers, which persists None when timezone is not specified. This contradicts the stated behavior ("defaults to UTC") and violates best practices for timezone storage. Pydantic's model default does not apply when None is explicitly passed.
Fix required in both code and test:
Code change in state-manager/app/tasks/trigger_cron.py line 74:
- timezone=trigger.timezone,
+ timezone=trigger.timezone or "UTC",Test change in state-manager/tests/unit/tasks/test_trigger_cron.py lines 20-22:
- # Verify None timezone is passed through (will default to UTC in ZoneInfo call)
- call_kwargs = mock_db_class.call_args[1]
- assert call_kwargs['timezone'] is None
+ # Verify None timezone is normalized and persisted as "UTC"
+ call_kwargs = mock_db_class.call_args[1]
+ assert call_kwargs['timezone'] == "UTC"Secondary concern: The unique index (lines 27-36 in trigger.py) omits timezone, which creates a collision risk when the same cron expression and trigger_time occur across different timezones.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pytest.mark.asyncio | |
| async def test_create_next_triggers_with_none_timezone_defaults_to_utc(): | |
| """Test create_next_triggers with None timezone defaults to UTC""" | |
| trigger = MagicMock() | |
| trigger.expression = "0 9 * * *" | |
| trigger.timezone = None | |
| trigger.trigger_time = datetime(2025, 10, 4, 9, 0, 0) | |
| trigger.graph_name = "test_graph" | |
| trigger.namespace = "test_namespace" | |
| cron_time = datetime(2025, 10, 6, 0, 0, 0) | |
| with patch('app.tasks.trigger_cron.DatabaseTriggers') as mock_db_class: | |
| mock_instance = MagicMock() | |
| mock_instance.insert = AsyncMock() | |
| mock_db_class.return_value = mock_instance | |
| await create_next_triggers(trigger, cron_time, 24) | |
| # Verify None timezone is passed through (will default to UTC in ZoneInfo call) | |
| call_kwargs = mock_db_class.call_args[1] | |
| assert call_kwargs['timezone'] is None | |
| @pytest.mark.asyncio | |
| async def test_create_next_triggers_with_none_timezone_defaults_to_utc(): | |
| """Test create_next_triggers with None timezone defaults to UTC""" | |
| trigger = MagicMock() | |
| trigger.expression = "0 9 * * *" | |
| trigger.timezone = None | |
| trigger.trigger_time = datetime(2025, 10, 4, 9, 0, 0) | |
| trigger.graph_name = "test_graph" | |
| trigger.namespace = "test_namespace" | |
| cron_time = datetime(2025, 10, 6, 0, 0, 0) | |
| with patch('app.tasks.trigger_cron.DatabaseTriggers') as mock_db_class: | |
| mock_instance = MagicMock() | |
| mock_instance.insert = AsyncMock() | |
| mock_db_class.return_value = mock_instance | |
| await create_next_triggers(trigger, cron_time, 24) | |
| # Verify None timezone is normalized and persisted as "UTC" | |
| call_kwargs = mock_db_class.call_args[1] | |
| assert call_kwargs['timezone'] == "UTC" |
| @pytest.mark.asyncio | ||
| async def test_create_next_triggers_with_europe_london_timezone(): | ||
| """Test create_next_triggers with Europe/London timezone""" | ||
| trigger = MagicMock() | ||
| trigger.expression = "0 17 * * *" | ||
| trigger.timezone = "Europe/London" | ||
| trigger.trigger_time = datetime(2025, 10, 4, 16, 0, 0) # UTC time | ||
| trigger.graph_name = "test_graph" | ||
| trigger.namespace = "test_namespace" | ||
|
|
||
| cron_time = datetime(2025, 10, 6, 0, 0, 0) | ||
|
|
||
| with patch('app.tasks.trigger_cron.DatabaseTriggers') as mock_db_class: | ||
| mock_instance = MagicMock() | ||
| mock_instance.insert = AsyncMock() | ||
| mock_db_class.return_value = mock_instance | ||
|
|
||
| await create_next_triggers(trigger, cron_time, 24) | ||
|
|
||
| # Verify Europe/London timezone was used | ||
| call_kwargs = mock_db_class.call_args[1] | ||
| assert call_kwargs['timezone'] == "Europe/London" | ||
|
|
||
| """ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add DST boundary and invalid timezone tests
To meet the PR objective on DST correctness, add:
- A fallback test (e.g., America/New_York, 2025-11-02, "0 1 * * *") asserting two distinct UTC times around the repeated 1 AM.
- An invalid timezone test asserting ZoneInfoNotFoundError.
Example additions (outside current hunks):
@pytest.mark.asyncio
async def test_create_next_triggers_dst_fall_back_repeated_hour():
trigger = MagicMock()
trigger.expression = "0 1 * * *"
trigger.timezone = "America/New_York"
trigger.trigger_time = datetime(2025, 11, 1, 9, 0, 0) # 5 AM EDT local baseline
trigger.graph_name = "test_graph"
trigger.namespace = "test_namespace"
cron_time = datetime(2025, 11, 3, 0, 0, 0)
with patch('app.tasks.trigger_cron.DatabaseTriggers') as mock_db:
inst = MagicMock(); inst.insert = AsyncMock(); mock_db.return_value = inst
await create_next_triggers(trigger, cron_time, 24)
# Expect two UTC times for 1 AM local (05:00Z and 06:00Z)
utc_times = [c.kwargs['trigger_time'] for c in mock_db.call_args_list]
assert any(t.hour == 5 for t in utc_times)
assert any(t.hour == 6 for t in utc_times)
@pytest.mark.asyncio
async def test_create_next_triggers_invalid_timezone_raises():
trigger = MagicMock()
trigger.expression = "0 9 * * *"
trigger.timezone = "Not/A_Real_TZ"
trigger.trigger_time = datetime(2025, 10, 4, 9, 0, 0)
trigger.graph_name = "test_graph"
trigger.namespace = "test_namespace"
with pytest.raises(Exception):
await create_next_triggers(trigger, datetime(2025, 10, 6, 0, 0, 0), 24)🤖 Prompt for AI Agents
In state-manager/tests/unit/tasks/test_trigger_cron.py around lines 98 to 121,
add two tests: one that verifies DST fall-back handling for America/New_York on
2025-11-02 by creating a trigger with expression "0 1 * * *", timezone
"America/New_York", trigger_time set to a UTC baseline just before the
transition, calling create_next_triggers for a multi-day window and asserting
the DatabaseTriggers inserts include two distinct UTC times corresponding to the
repeated 1 AM local hour (e.g., one at 05:00Z and one at 06:00Z); and another
test that supplies an invalid timezone (e.g., "Not/A_Real_TZ") and asserts
create_next_triggers raises the appropriate ZoneInfoNotFoundError (or the
specific exception your code raises for unknown zones), patching
DatabaseTriggers in both tests and using AsyncMock for insert.
| async def test_create_next_triggers_handles_duplicate_key_error(): | ||
| """Test create_next_triggers handles DuplicateKeyError gracefully""" | ||
| cron_time = datetime.now(timezone.utc) | ||
| trigger = MagicMock(spec=DatabaseTriggers) | ||
| trigger = MagicMock() | ||
| trigger.expression = "0 9 * * *" | ||
| trigger.trigger_time = cron_time - timedelta(days=1) | ||
| trigger.timezone = "America/New_York" | ||
| trigger.trigger_time = datetime(2025, 10, 4, 13, 0, 0) | ||
| trigger.graph_name = "test_graph" | ||
| trigger.namespace = "test_ns" | ||
| trigger.namespace = "test_namespace" | ||
|
|
||
| with patch('app.tasks.trigger_cron.DatabaseTriggers') as MockDatabaseTriggers: | ||
| cron_time = datetime(2025, 10, 6, 0, 0, 0) | ||
|
|
||
| with patch('app.tasks.trigger_cron.DatabaseTriggers') as mock_db_class: | ||
| mock_instance = MagicMock() | ||
| mock_instance.insert = AsyncMock(side_effect=DuplicateKeyError("duplicate")) | ||
| MockDatabaseTriggers.return_value = mock_instance | ||
| # First call raises DuplicateKeyError, second succeeds | ||
| mock_instance.insert = AsyncMock(side_effect=[ | ||
| DuplicateKeyError("Duplicate"), | ||
| None | ||
| ]) | ||
| mock_db_class.return_value = mock_instance | ||
|
|
||
| with patch('app.tasks.trigger_cron.logger') as mock_logger: | ||
| # Should not raise exception | ||
| await create_next_triggers(trigger, cron_time, 24) | ||
|
|
||
| # Verify error was logged | ||
| assert mock_logger.error.called | ||
| error_msg = mock_logger.error.call_args[0][0] | ||
| assert "Duplicate trigger found" in error_msg | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Also verify we continue after DuplicateKeyError
Ensure we still insert subsequent triggers after logging the duplicate.
# Verify error was logged
assert mock_logger.error.called
error_msg = mock_logger.error.call_args[0][0]
assert "Duplicate trigger found" in error_msg
+ # And insertion continued after the duplicate
+ assert mock_instance.insert.call_count >= 2🤖 Prompt for AI Agents
In state-manager/tests/unit/tasks/test_trigger_cron.py around lines 281 to 309,
the test logs DuplicateKeyError but does not assert we continued inserting
subsequent triggers; after the existing assertions, add a check that the insert
method was called for the second trigger (e.g., assert
mock_instance.insert.call_count == 2 or assert mock_instance.insert.await_count
== 2) and optionally verify the args of the second call match the expected
trigger for the next time slot to ensure processing continued after the
DuplicateKeyError.
| @pytest.mark.asyncio | ||
| async def test_create_next_triggers_trigger_time_is_datetime(): | ||
| """Test that next trigger_time is a datetime object""" | ||
| trigger = MagicMock() | ||
| trigger.expression = "0 9 * * *" | ||
| trigger.timezone = "America/New_York" | ||
| trigger.trigger_time = datetime(2025, 10, 4, 13, 0, 0) | ||
| trigger.graph_name = "test_graph" | ||
| trigger.namespace = "test_namespace" | ||
|
|
||
| cron_time = datetime(2025, 10, 6, 0, 0, 0) | ||
|
|
||
| with patch('app.tasks.trigger_cron.DatabaseTriggers') as mock_db_class: | ||
| mock_instance = MagicMock() | ||
| mock_instance.insert = AsyncMock() | ||
| mock_db_class.return_value = mock_instance | ||
|
|
||
| # Should not raise exception | ||
| await create_next_triggers(trigger, cron_time, 24) | ||
|
|
||
| # Verify trigger_time is a datetime | ||
| call_kwargs = mock_db_class.call_args[1] | ||
| assert isinstance(call_kwargs['trigger_time'], datetime) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Strengthen invariants: stored datetimes are UTC-naive
create_next_triggers stores trigger_time/expires_at as UTC-naive datetimes. Assert tzinfo is None and expires_at is a datetime to guard regressions.
await create_next_triggers(trigger, cron_time, 24)
# Verify trigger_time is a datetime
call_kwargs = mock_db_class.call_args[1]
assert isinstance(call_kwargs['trigger_time'], datetime)
+ # Stored in UTC as naive datetime
+ assert call_kwargs['trigger_time'].tzinfo is None
+ # Also validate expires_at presence and type
+ assert isinstance(call_kwargs['expires_at'], datetime)
+ assert call_kwargs['expires_at'].tzinfo is None🤖 Prompt for AI Agents
In state-manager/tests/unit/tasks/test_trigger_cron.py around lines 311 to 333,
the test currently only asserts that the stored trigger_time is a datetime;
strengthen the invariant by also asserting that trigger_time.tzinfo is None
(UTC-naive) and that expires_at is present and is a datetime with tzinfo None.
Update the test to capture the actual arguments used when the DatabaseTriggers
insert was called (from the mock instance or mock_class call args), then add
assertions: isinstance(trigger_time, datetime), trigger_time.tzinfo is None,
isinstance(expires_at, datetime), and expires_at.tzinfo is None.
| @pytest.mark.asyncio | ||
| async def test_create_next_triggers_creates_multiple_triggers(): | ||
| """Test create_next_triggers creates multiple future triggers""" | ||
| trigger = MagicMock() | ||
| trigger.expression = "0 */6 * * *" # Every 6 hours | ||
| trigger.timezone = "UTC" | ||
| trigger.trigger_time = datetime(2025, 10, 4, 0, 0, 0) | ||
| trigger.graph_name = "test_graph" | ||
| trigger.namespace = "test_namespace" | ||
|
|
||
| cron_time = datetime(2025, 10, 5, 0, 0, 0) # 24 hours later | ||
|
|
||
| with patch('app.tasks.trigger_cron.DatabaseTriggers') as mock_db_class: | ||
| mock_instance = MagicMock() | ||
| mock_instance.insert = AsyncMock() | ||
| mock_db_class.return_value = mock_instance | ||
|
|
||
| await create_next_triggers(trigger, cron_time, 24) | ||
|
|
||
| # Should create multiple triggers (every 6 hours until past cron_time) | ||
| assert mock_db_class.call_count >= 4 # At least 4 triggers in 24 hours | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: compute expected creation count programmatically
Instead of asserting “>= 4”, derive the expected count using croniter until > cron_time to catch regressions precisely. This keeps the test independent of hardcoded boundaries while remaining deterministic.
# Pseudocode inside the test:
from zoneinfo import ZoneInfo
import croniter
tz = ZoneInfo(trigger.timezone or "UTC")
base = trigger.trigger_time.replace(tzinfo=timezone.utc).astimezone(tz)
it = croniter.croniter(trigger.expression, base)
expected = 0
while True:
t = it.get_next(datetime).astimezone(timezone.utc).replace(tzinfo=None)
expected += 1
if t > cron_time:
break
assert mock_db_class.call_count == expected🤖 Prompt for AI Agents
In state-manager/tests/unit/tasks/test_trigger_cron.py around lines 335 to 356,
replace the hardcoded assertion that checks for ">= 4" created triggers with a
programmatic calculation of the expected creation count using croniter and
proper timezone handling: instantiate a croniter with the trigger.expression and
trigger.trigger_time converted into the trigger.timezone (falling back to UTC),
iterate get_next(datetime) converting results to UTC naive datetimes until a
generated time is > cron_time, count iterations, then assert
mock_db_class.call_count == expected to make the test deterministic and
resilient to expression changes.
2 participants
Allow users to specify timezone when setting up CRON triggers to enable scheduling in local time zones with automatic DST handling.
Changes:
The timezone field is optional and defaults to "UTC". All trigger times are internally stored in UTC while croniter calculations respect the specified timezone, ensuring correct scheduling across time zones and DST transitions.
Closes #438