Conversation
📝 WalkthroughWalkthroughUpdated time handling to use timezone-aware UTC now() in runtime logic and corresponding tests: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/operator/backend_listener.py (2)
933-941:⚠️ Potential issue | 🔴 CriticalFix timezone mismatch causing pipeline failure.
The change to use
datetime.datetime.now(datetime.timezone.utc)is correct in intent, butcondition.last_transition_timefrom the Kubernetes API may be timezone-naive, causing the subtraction at line 936 to fail withTypeError: can't subtract offset-naive and offset-aware datetimes.You need to ensure
last_transition_timeis timezone-aware before comparison.🐛 Proposed fix to handle timezone mismatch
if condition.type == 'Ready' and condition.status == 'False': now = datetime.datetime.now(datetime.timezone.utc) last_transition_time = condition.last_transition_time if last_transition_time: + # Ensure timezone-aware comparison + if last_transition_time.tzinfo is None: + last_transition_time = last_transition_time.replace( + tzinfo=datetime.timezone.utc) time_diff = now - last_transition_timeNote: The same issue likely exists at lines 952-955 for the
ContainerStatusUnknownhandling, which uses the same pattern. Consider applying the same fix there for consistency and safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/operator/backend_listener.py` around lines 933 - 941, condition.last_transition_time from the k8s API can be timezone-naive, causing TypeError when subtracting from datetime.now(timezone.utc); update the check around now = datetime.datetime.now(datetime.timezone.utc) and time_diff = now - last_transition_time (the block that sets task.TaskGroupStatus.FAILED_BACKEND_ERROR and exit_code = task.ExitCode.FAILED_BACKEND_ERROR.value) to first coerce last_transition_time to an aware timestamp (e.g., if last_transition_time.tzinfo is None then treat it as UTC by attaching datetime.timezone.utc or convert it) before computing time_diff, and apply the same fix in the ContainerStatusUnknown handling that uses the same pattern so both comparisons are timezone-aware and safe.
952-960:⚠️ Potential issue | 🟠 MajorSame timezone mismatch vulnerability exists here.
This code block has the identical pattern and will fail with the same
TypeErroriflast_transition_timeis timezone-naive. Apply the same timezone handling fix for consistency.🐛 Proposed fix
if condition.type == 'Ready' and condition.status == 'False': now = datetime.datetime.now(datetime.timezone.utc) last_transition_time = condition.last_transition_time if last_transition_time: + # Ensure timezone-aware comparison + if last_transition_time.tzinfo is None: + last_transition_time = last_transition_time.replace( + tzinfo=datetime.timezone.utc) time_diff = now - last_transition_time🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/operator/backend_listener.py` around lines 952 - 960, The same timezone-naive vulnerability appears when computing time_diff using now = datetime.datetime.now(datetime.timezone.utc) and condition.last_transition_time; ensure last_transition_time is made timezone-aware before subtracting: check condition.last_transition_time.tzinfo (or use hasattr) and if it's naive, set its tzinfo to datetime.timezone.utc (e.g., via replace) so the subsequent time_diff > datetime.timedelta(minutes=30) comparison is between two aware datetimes; update the block that sets status = task.TaskGroupStatus.FAILED_BACKEND_ERROR and exit_code = task.ExitCode.FAILED_BACKEND_ERROR.value to use this normalized last_transition_time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/operator/backend_listener.py`:
- Around line 933-941: condition.last_transition_time from the k8s API can be
timezone-naive, causing TypeError when subtracting from
datetime.now(timezone.utc); update the check around now =
datetime.datetime.now(datetime.timezone.utc) and time_diff = now -
last_transition_time (the block that sets
task.TaskGroupStatus.FAILED_BACKEND_ERROR and exit_code =
task.ExitCode.FAILED_BACKEND_ERROR.value) to first coerce last_transition_time
to an aware timestamp (e.g., if last_transition_time.tzinfo is None then treat
it as UTC by attaching datetime.timezone.utc or convert it) before computing
time_diff, and apply the same fix in the ContainerStatusUnknown handling that
uses the same pattern so both comparisons are timezone-aware and safe.
- Around line 952-960: The same timezone-naive vulnerability appears when
computing time_diff using now = datetime.datetime.now(datetime.timezone.utc) and
condition.last_transition_time; ensure last_transition_time is made
timezone-aware before subtracting: check condition.last_transition_time.tzinfo
(or use hasattr) and if it's naive, set its tzinfo to datetime.timezone.utc
(e.g., via replace) so the subsequent time_diff > datetime.timedelta(minutes=30)
comparison is between two aware datetimes; update the block that sets status =
task.TaskGroupStatus.FAILED_BACKEND_ERROR and exit_code =
task.ExitCode.FAILED_BACKEND_ERROR.value to use this normalized
last_transition_time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 044b4958-d17d-4d25-90f2-883927bd9d5d
📒 Files selected for processing (1)
src/operator/backend_listener.py
ethany-nv
left a comment
ethany-nv
left a comment
There was a problem hiding this comment.
Please fix title and description
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/operator/tests/test_pod_status_calculator.py (1)
36-49: LGTM! Timezone-aware UTC datetime changes are consistent.The refactoring correctly ensures all code paths return timezone-aware UTC datetimes, aligning with the production code changes in
backend_listener.py.One minor consideration: the fallback on line 49 silently returns
utc_nowfor invalid format strings (e.g.,"now-5d"or malformed input). Since this is a test helper, you might want to raise an error for unsupported formats to catch test configuration mistakes early.💡 Optional: Add explicit error for unsupported formats
elif parts.endswith('h'): hours = int(parts[:-1]) return utc_now - datetime.timedelta(hours=hours) - return utc_now + else: + raise ValueError(f"Unsupported time format: {time_str}") + raise ValueError(f"Unsupported time format: {time_str}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/operator/tests/test_pod_status_calculator.py` around lines 36 - 49, The helper parse_time_string currently falls back to returning utc_now for unknown or malformed inputs; instead change it to raise a clear ValueError for unsupported formats so test misconfiguration fails fast. In parse_time_string, after handling 'now', 'now-<n>m', and 'now-<n>h' cases, replace the final return utc_now with raising ValueError(f"Unsupported time string: {time_str}"), and ensure any int conversion/parsing paths propagate errors (or convert parse errors into the same ValueError) so inputs like "now-5d" or malformed strings trigger a test failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/operator/tests/test_pod_status_calculator.py`:
- Around line 36-49: The helper parse_time_string currently falls back to
returning utc_now for unknown or malformed inputs; instead change it to raise a
clear ValueError for unsupported formats so test misconfiguration fails fast. In
parse_time_string, after handling 'now', 'now-<n>m', and 'now-<n>h' cases,
replace the final return utc_now with raising ValueError(f"Unsupported time
string: {time_str}"), and ensure any int conversion/parsing paths propagate
errors (or convert parse errors into the same ValueError) so inputs like
"now-5d" or malformed strings trigger a test failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4b0e1931-4749-49a3-8702-0711632884ec
📒 Files selected for processing (1)
src/operator/tests/test_pod_status_calculator.py
4 participants
Description
Issue #None
Checklist
Summary by CodeRabbit