UN-2454 [FIX] Avoid polling infinitely for in-progress execution #1630
Conversation
- Update DetailedLogs and ExecutionLogs components to properly handle polling state - Update index.js dependencies - Update workers dependencies in pyproject.toml and uv.lock 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Summary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds refs-based polling and cleanup to DetailedLogs and ExecutionLogs; introduces shouldPoll (EXECUTING/PENDING + one-hour freshness using createdAtRaw), surfaces createdAtRaw/totalFiles/workflowName/pipelineName from fetches, and centralizes interval/timeout management with cleanup on dependency changes and unmount. Changes
Sequence Diagram(s)sequenceDiagram participant UI as Component (DetailedLogs / ExecutionLogs) participant Fetch as fetchExecutionDetails / fetchExecutionFiles / fetchLogs participant Gate as shouldPoll() participant Timer as Interval/Timeout Ref UI->>Fetch: initial fetch (include createdAtRaw, totalFiles, names) Fetch-->>UI: data (items / executionDetails with createdAtRaw) UI->>Gate: evaluate status + createdAtRaw alt shouldPoll == true Gate-->>UI: allow polling UI->>Timer: start interval/timeout (store ref) loop Poll cycles (every 5s or per-item timeout) Timer->>Fetch: poll update for item/execution Fetch-->>UI: updated item/details UI->>Gate: re-evaluate status + createdAtRaw alt continue polling Gate-->>Timer: keep timer/timeout else stop polling Gate-->>UI: stop polling UI->>Timer: clear ref (clearPolling / clearAllPolling) end end else shouldPoll == false Gate-->>UI: no polling started end UI->>Timer: on unmount / navigate / tab change Timer-->>UI: clear timers/timeouts Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
chandrasekharan-zipstack requested review from athul-rs, jagadeeswaran-zipstack and vishnuszipstack 
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
Reverted files that were not part of the intended fix: - frontend/src/index.js - workers/pyproject.toml - workers/uv.lock 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx(3 hunks)frontend/src/components/logging/execution-logs/ExecutionLogs.jsx(5 hunks)
- Fixed date calculation bug: use raw ISO timestamps instead of formatted display strings - Added createdAtRaw and modified_atRaw fields to preserve parseable date values - Implement stale interval re-check: stop polling when execution is >1 hour old via executionDetailsRef - Replace forEach loops with for...of for improved performance (4 instances) Fixes CodeRabbit comments about infinite polling and ensures logging components correctly handle timestamp calculations and respect staleness thresholds. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (2)
168-179: LGTM!The refactoring to
for...ofloops is cleaner and more idiomatic. No functional changes.
357-360: Remove redundant unmount cleanup effect.The effect at lines 334-355 already returns
clearPollingin its cleanup function, which runs on unmount. This separate empty-dependency effect is redundant—both cleanup functions will callclearPolling()when the component unmounts. While safe (due to the existence check inclearPolling), it adds unnecessary code.Apply this diff to remove the duplicate:
- // Clear polling when component unmounts - useEffect(() => { - return clearPolling; - }, []); -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx(5 hunks)frontend/src/components/logging/execution-logs/ExecutionLogs.jsx(6 hunks)
🔇 Additional comments (12)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (5)
1-1: LGTM!The addition of
useRefand the two refs for polling management is appropriate for tracking interval IDs and latest execution state across renders.Also applies to: 64-67
95-101: LGTM!The
clearPollingimplementation correctly clears the interval and resets the ref. The existence check makes it safe to call multiple times.
103-134: LGTM!Adding
createdAtRawat line 113 correctly preserves the raw ISO timestamp for staleness checks, addressing previous review feedback.
329-332: LGTM!This effect correctly keeps
executionDetailsRefin sync with the latestexecutionDetailsstate, enabling mid-interval staleness checks as recommended in previous reviews.
334-355: LGTM!The polling effect correctly:
- Clears existing intervals before creating new ones
- Re-checks staleness mid-interval using
executionDetailsRef.current- Cleans up on dependency changes and unmount
This addresses the past review feedback about re-evaluating the staleness check during the polling cycle.
frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (7)
3-3: LGTM!Adding
useRefandpollingTimeoutsReffor tracking multiple timeout IDs is appropriate. The object structure allows efficient keyed cleanup by execution ID.Also applies to: 47-48
123-138: LGTM!Both
clearPollingTimeoutandclearAllPollingcorrectly manage timeout cleanup. The existence checks and state resets are properly implemented.
140-202: LGTM!
pollExecutingRecordcorrectly:
- Populates
createdAtRawfor staleness checks (line 162)- Uses
shouldPollto determine when to stop polling (lines 174-175)- Cleans up timeouts when polling stops (line 181)
- Stores new timeout IDs in the ref for proper lifecycle management (line 189)
204-215: LGTM!
startPollingForExecutingcorrectly usesshouldPollto filter records and prevents duplicate polling with thepollingIdscheck. Thefor...ofloop is clean and idiomatic.
217-267: LGTM!Line 243 correctly populates
createdAtRawwith the raw ISO timestamp from the API response, enabling accurate staleness checks.
296-299: LGTM!The effect correctly clears all polling when the view changes (
idoractiveTab) or when the component unmounts, preventing memory leaks and stale polling.
301-308: LGTM!Line 304 correctly clears all existing polling before fetching new logs, ensuring that timeouts for records no longer in the view are properly cleaned up.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Chandrasekharan M <117059509+chandrasekharan-zipstack@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (2)
170-174: Good loop improvements (incidental refactor).The changes from
for...intofor...ofwithObject.entries()and array iteration are more idiomatic and avoid potential issues with inherited properties, though they're not directly related to the PR's polling lifecycle goal.Also applies to: 178-180
359-362: Optional: Redundant unmount cleanup.This cleanup effect is redundant with the cleanup returned by the polling effect (line 356), which already runs on unmount. While harmless (since
clearPolling()is idempotent), this could be removed for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (6)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (6)
1-1: LGTM! Proper ref-based interval management.The addition of
useRefand the two refs (pollingIntervalReffor interval cleanup andexecutionDetailsReffor mid-cycle freshness checks) follows React best practices for managing side effects with intervals.Also applies to: 64-67
71-95: LGTM! Previous critical validation issues now resolved.The
shouldPoll()implementation correctly:
- Validates all inputs before computing staleness
- Handles Invalid Date scenarios with
isNaN(createdAt.getTime())- Uses the raw ISO timestamp for accurate time calculations
- Stops polling for non-EXECUTING/PENDING statuses or executions older than 1 hour
This addresses the previous critical issues raised about timestamp parsing and validation failures.
97-103: LGTM! Clean interval cleanup utility.The
clearPolling()function is idempotent and properly resets the ref, making it safe to call from multiple cleanup paths.
115-115: LGTM! Raw timestamp preserved for accurate staleness checks.Adding
createdAtRawpreserves the ISO timestamp from the backend, enabling reliable Date parsing and time calculations inshouldPoll(). This resolves the previous critical issue with formatted string parsing.
331-334: LGTM! Ref synchronization enables fresh staleness checks.This effect ensures
executionDetailsRefalways holds the latest execution details, allowing the polling interval to re-evaluate staleness mid-cycle using fresh data.
336-357: LGTM! Robust polling lifecycle with mid-cycle staleness detection.The polling implementation correctly:
- Clears any existing interval before starting a new one (prevents overlaps)
- Re-checks
shouldPoll()inside the interval callback using the latest execution details from the ref- Stops polling mid-cycle when the 1-hour threshold is crossed
- Cleans up on dependency changes and unmount
This addresses the previous critical issue where intervals continued running past the 1-hour mark.
…mponents Replace isNaN() with isFinite() for date validation in ExecutionLogs and DetailedLogs to prevent infinite polling when invalid timestamps occur. - DetailedLogs.jsx: Changed isNaN() to isFinite() check - ExecutionLogs.jsx: Added missing isFinite() date validation 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx(5 hunks)frontend/src/components/logging/execution-logs/ExecutionLogs.jsx(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
Use ref instead of state for pollingIds to prevent stale closure issues in polling logic: - Added pollingIdsRef to track actively polling execution IDs - Updated all polling operations to directly mutate ref (no re-renders needed) - Removed redundant pollingIds state to eliminate unnecessary re-renders - Prevents duplicate polling loops and potential memory leaks 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
jagadeeswaran-zipstack left a comment
jagadeeswaran-zipstack left a comment There was a problem hiding this comment.
Please address the minor changes requested.
chandrasekharan-zipstack changed the title chandrasekharan-zipstack changed the title 
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (1)
424-427: Potential TypeError ifstatusis undefined.
executionDetails?.status.toLowerCase()will throw ifstatusisnullorundefinedbecause optional chaining doesn't guard the subsequent.toLowerCase()call.Apply this diff to add defensive access:
- {executionDetails?.status.toLowerCase() === "executing" + {executionDetails?.status?.toLowerCase() === "executing" ? "Running for" : "Ran for"}The same pattern should be applied on line 437.
🧹 Nitpick comments (2)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (1)
359-362: Consider removing redundant unmount cleanup effect.The cleanup function returned on line 356 already runs on unmount (React invokes cleanup when the component unmounts, regardless of which dependencies triggered it). This separate effect duplicates that behavior.
You can safely remove this effect:
- // Clear polling when component unmounts - useEffect(() => { - return clearPolling; - }, []);frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (1)
100-123: Timestamp validation correctly handles invalid dates.The
Number.isFinite(createdAt.getTime())check properly addresses the past comment about invalid timestamps. WhencreatedAtRawandcreated_atare both missing, this returnsfalseand stops polling as expected.Consider guarding against future timestamps.
If
createdAtis in the future (due to clock skew or invalid data),timeDifferencewould be negative and the 1-hour check would never trigger, allowing indefinite polling. Consider adding a check:const timeDifference = now - createdAt; + + // Guard against future timestamps + if (timeDifference < 0) return false; if (timeDifference > oneHourInMs) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx(5 hunks)frontend/src/components/logging/execution-logs/ExecutionLogs.jsx(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (14)
frontend/src/components/logging/detailed-logs/DetailedLogs.jsx (6)
64-67: Good use of refs for polling state management.Using
useReffor the interval ID and latest execution details is the correct pattern for managing mutable values that need to persist across renders and be accessible in interval callbacks without causing stale closure issues.
71-95: Well-implemented polling eligibility check.The
shouldPollfunction correctly validates:
- Execution details exist
- Status is EXECUTING or PENDING
- Timestamp is present and valid (using
Number.isFinite)- Execution is not stale (< 1 hour old)
The early returns for invalid states ensure defensive behavior.
97-103: LGTM!The
clearPollinghelper properly clears the interval and nullifies the ref to prevent duplicate cleanup attempts.
114-128: Correct fix for timestamp parsing.Adding
createdAtRawpreserves the original ISO timestamp from the API, enabling reliable date parsing inshouldPoll. This addresses the previous review feedback about formatted display strings causingInvalid Date.
331-334: LGTM!Keeping
executionDetailsRefsynchronized with the latestexecutionDetailsstate ensures the interval callback always has access to current data, avoiding stale closure issues.
336-357: Polling logic correctly addresses previous review feedback.The implementation now:
- Clears existing intervals before setting new ones (prevents leaks)
- Re-checks staleness inside the interval callback using the ref
- Returns cleanup function for dependency changes
One consideration: the
pagination.currentdependency means the polling interval restarts on every page change. This is likely intentional to fetch the correct page, but be aware it couples pagination navigation with polling lifecycle.frontend/src/components/logging/execution-logs/ExecutionLogs.jsx (8)
3-3: LGTM!The addition of
useRefto the imports is correct and necessary for the polling lifecycle management implemented in this PR.
46-49: LGTM!Using refs to store polling timeouts and IDs is the correct approach to avoid stale closure issues and prevent unnecessary re-renders during polling updates.
125-131: LGTM!The
clearPollingTimeoutfunction correctly clears the timeout and removes the entry from the ref, preventing memory leaks.
133-141: LGTM!The
clearAllPollingfunction correctly clears all active timeouts and resets both refs, ensuring thorough cleanup when switching views or unmounting.
143-199: LGTM!The
pollExecutingRecordfunction correctly:
- Updates the record in
dataListwith fresh execution data- Uses
shouldPollto determine whether to continue polling- Synchronizes
pollingIdsRefby removing IDs when polling stops- Cleans up timeouts when polling ends or on error
- Schedules the next poll with a 5-second delay for active executions
This implementation properly addresses the past comment about stale closures by maintaining ref synchronization.
201-209: LGTM!The
startPollingForExecutingfunction correctly:
- Uses
pollingIdsRefto track which records are already being polled- Adds the record key to the ref before initiating polling to prevent race conditions
- Only starts polling for records that pass the
shouldPollcheckThis implementation avoids the stale closure issue mentioned in past comments by using refs instead of state.
211-261: LGTM!The
fetchLogsfunction correctly:
- Maps
createdAtRawfrom the API response (line 237) to enable staleness checks inshouldPoll- Initiates polling for executing/pending records after updating
dataList(line 255)- Works in conjunction with
clearAllPolling(called before fetch in the useEffect) to prevent stale polling
290-302: LGTM!The cleanup logic correctly:
- Clears all polling on component unmount or when
id/activeTabchanges (lines 290-293)- Clears polling before fetching new logs to prevent stale polls (line 298)
The
activeTabdependency appears in bothuseEffecthooks, causingclearAllPollingto be called twice when the tab changes. This is safe (idempotent cleanup) but creates slight redundancy.
What
Why
How
Frontend Changes (DetailedLogs.jsx):
useRefto store polling interval ID for proper cleanupshouldPoll()function to check execution status and ageclearPolling()to properly cleanup intervalsFrontend Changes (ExecutionLogs.jsx):
useRefto store polling timeout IDs for multiple executionsshouldPoll()usingmodified_attimestampclearPollingTimeout()andclearAllPolling()for cleanupCan this PR break any existing features. If yes, please list possible items. If no, please explain why.
No, this PR does not break existing features:
Related Issues or PRs
Notes on Testing
Checklist
I have read and understood the Contribution Guidelines.
🤖 Generated with Claude Code