fix: bound live UI reads and DAG run timeouts#1827
Merged
yottahmd merged 4 commits intodagucloud:mainfrom Mar 22, 2026
Merged
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughIntroduces context-aware cancellation through the request pipeline: backend persistence layer propagates Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as API Handler
participant Persist as Persistence Layer
participant Cache as Status Cache
Client->>API: ReadStatus(ctx with timeout)
API->>Persist: FindByDAGRunID(ctx)
Persist->>Persist: Check ctx.Err() during iteration
alt Context Cancelled
Persist-->>API: context.Canceled
else Context Valid
Persist->>Persist: LatestAttempt(ctx)
Persist->>Persist: Check ctx.Err() in loop
Persist->>Persist: parseLocked(ctx)
Persist->>Cache: Check cached status
alt Cache Miss
Persist->>Persist: parseStatusFileWithContext(ctx)
Persist->>Persist: Check ctx.Err() during scan
else Cache Hit
Persist->>Persist: parseLocked(ctx) on cached data
end
Persist-->>API: *DAGRunStatus
end
API-->>Client: Response or timeout error
sequenceDiagram
participant Component as React Component
participant Hook as useBoundedDAGRunDetails
participant Timer as Polling Timer
participant Fetch as fetchWithTimeout
participant API as Backend API
Component->>Hook: useBoundedDAGRunDetails(target, enabled)
activate Hook
Hook->>Hook: Initialize refs (inFlight, pending)
Hook->>Hook: Schedule initial fetch
deactivate Hook
Loop Polling (when enabled & pollIntervalMs > 0)
Timer->>Hook: Poll interval elapsed
Hook->>Hook: Check inFlightRef (prevent overlap)
alt Request In Flight
Hook->>Hook: Queue in pendingRef
else No Request In Flight
Hook->>Hook: Create AbortController
Hook->>Fetch: fetchWithTimeout(target)
Fetch->>Fetch: Link abort signals, set timeout
Fetch->>API: fetch(request, {signal})
alt Timeout or Abort
API-->>Fetch: AbortSignal fired
Fetch-->>Hook: RequestTimeoutError or RequestAbortError
else Success
API-->>Fetch: Response
Fetch-->>Hook: data (mapped)
end
Hook->>Hook: Update state (data, error, isValidating)
alt Pending Refetch Queued
Hook->>Hook: Clear pending, reschedule polling
end
end
end
Component->>Hook: Call refresh()
Hook->>Fetch: fetchWithTimeout(target, {signal: abortController.signal})
Fetch->>API: fetch(request, {signal})
API-->>Fetch: Response
Fetch-->>Hook: DAGRunDetails
Hook->>Hook: Update data, clear error
Hook-->>Component: Refreshed details
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
ui/src/features/dag-runs/hooks/dagRunDetailsRequest.ts (1)
1-4: Missing GPL v3 license header.As per coding guidelines, source files should have GPL v3 license headers managed via
make addlicense.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/dag-runs/hooks/dagRunDetailsRequest.ts` around lines 1 - 4, This file is missing the required GPL v3 license header; add the standard GPL v3 license header to the top of the file (above the imports) by running the repository tool: execute make addlicense so the header is applied automatically, ensuring it precedes the existing imports and type export (references: the import lines for components and fetchJson and the exported type DAGRunDetails).ui/src/features/dag-runs/hooks/useBoundedDAGRunDetails.ts (2)
188-196: RedundantmountedRef.current = trueassignment.The ref is already initialized to
trueon line 48, and refs persist across renders. The assignment on line 189 is unnecessary.🧹 Proposed cleanup
useEffect(() => { - mountedRef.current = true; return () => { mountedRef.current = false; clearPollTimer(); abortActiveRequest(); pendingRef.current = false; }; }, [abortActiveRequest, clearPollTimer]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/dag-runs/hooks/useBoundedDAGRunDetails.ts` around lines 188 - 196, In useBoundedDAGRunDetails, remove the redundant mountedRef.current = true assignment inside the useEffect since mountedRef is already initialized to true and refs persist; keep the cleanup callback that sets mountedRef.current = false and calls clearPollTimer(), abortActiveRequest(), and sets pendingRef.current = false, leaving the effect dependencies ([abortActiveRequest, clearPollTimer]) unchanged.
1-8: Missing GPL v3 license header.As per coding guidelines, source files should have GPL v3 license headers managed via
make addlicense.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/dag-runs/hooks/useBoundedDAGRunDetails.ts` around lines 1 - 8, This file lacks the required GPL v3 license header; run the project's license tooling to add it (e.g., execute the make addlicense target) so the header is inserted at the top of the module containing useBoundedDAGRunDetails (the file that imports fetchDAGRunDetails and isAbortLikeError and declares useBoundedDAGRunDetails). Ensure the header matches the project's GPL v3 template used by addlicense and is placed before any imports or code.ui/src/lib/requestTimeout.ts (1)
1-6: Missing GPL v3 license header.As per coding guidelines, source files should have GPL v3 license headers managed via
make addlicense.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/lib/requestTimeout.ts` around lines 1 - 6, This file is missing the required GPL v3 license header; add the standard GPL v3 header to the top of the file (above the existing declarations like READ_METHODS, DEFAULT_READ_TIMEOUT_MS, DEFAULT_WRITE_TIMEOUT_MS, and LIVE_INVALIDATION_COOLDOWN_MS) by running the repository's license tool (make addlicense) or inserting the project's standard header so the file complies with the project's licensing guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/service/frontend/api/v1/dagruns.go`:
- Around line 1359-1371: The handler currently only checks for
context.DeadlineExceeded from withDAGRunReadTimeout and treats other errors
(including context.Canceled) as not-found; update the error handling in
GetDAGRunDetails (the block that calls withDAGRunReadTimeout and uses
dagRunReadRequestInfo) to first detect errors.Is(err, context.Canceled) and
return an appropriate "request canceled" response (same style as the deadline
branch — e.g., a short-circuit response with a client-canceled status/body)
instead of falling through to the 404 branch; apply the identical change to the
GetSubDAGRunDetails handler so canceled contexts are preserved there as well.
- Around line 82-100: The helper currently returns a successful result if
err==nil even when the child context (readCtx) has already been cancelled or hit
its deadline; after calling read(readCtx) and before returning result, check
readCtx.Err() and if it is context.DeadlineExceeded or context.Canceled handle
it the same way as in the switch: log via logger.Warn with info.attrs(duration)
and return a zero T plus the corresponding context error (use startedAt to
compute duration). In short: after obtaining result, inspect readCtx.Err() and
convert late-success into the appropriate timeout/cancel error instead of
returning result.
In `@ui/src/features/dag-runs/components/dag-run-details/DAGRunDetailsModal.tsx`:
- Around line 103-105: The check for freshDetails uses expectedDagRunId which
equals 'latest' for dagRunId === 'latest', but the server returns a resolved
UUID so equality fails; update the logic in DAGRunDetailsModal.tsx to consider a
resolved response as fresh when the requested dagRunId was the sentinel 'latest'
(or when canQuerySubDag and subDAGRunId is used) — e.g., detect if dagRunId ===
'latest' and then accept latestDetails as fresh when latestDetails?.dagRunId is
defined (or compare against latestDetails?.dagRunId instead of the literal
'latest'); adjust the assignment of expectedDagRunId/freshDetails (references:
expectedDagRunId, canQuerySubDag, subDAGRunId, dagRunId, latestDetails,
freshDetails) so the first open shows the returned details for 'latest'.
In `@ui/src/features/dag-runs/components/dag-run-details/DAGRunDetailsPanel.tsx`:
- Around line 63-67: The current guard compares latestDetails?.dagRunId to
expectedDagRunId which can be the literal 'latest', so when the API returns a
concrete DAG run id for a "latest" request data stays null; change the match
logic so that when the requested dagRunId is 'latest' (or dagRunId is falsy and
you defaulted to 'latest') you treat any returned latestDetails as a match.
Concretely, update the conditional that sets data (using expectedDagRunId,
isSubDAGRun, subDAGRunId, dagRunId, and latestDetails?.dagRunId) to also
consider expectedDagRunId === 'latest' as a successful match (i.e., set data = {
dagRunDetails: latestDetails } if latestDetails exists and either
latestDetails.dagRunId === expectedDagRunId OR expectedDagRunId === 'latest').
In `@ui/src/features/dag-runs/components/dag-run-list/StepDetailsTooltip.tsx`:
- Around line 109-117: The tooltip clears details then sets isLoading false
before the hover delay, causing a “No running…” flash and allowing an older
aborted request to hide the spinner for a newer one; fix by setting
setIsLoading(true) immediately when scheduling the fetch (before/when calling
setTimeout via fetchTimerRef) so the loading state shows during the delay, and
in the async fetch handler use a local controller variable (const controller =
new AbortController(); controllerRef.current = controller) and only call
setIsLoading(false) in finally if controllerRef.current === controller to avoid
older requests clearing the spinner; apply the same change to the corresponding
block around lines 143-147.
In `@ui/src/features/dag-runs/hooks/useBoundedDAGRunDetails.ts`:
- Around line 129-153: The finally block currently contains return statements
which can suppress thrown exceptions; remove those returns and instead capture
necessary post-finally actions with local flags so the control flow happens
after the try/catch/finally completes. Specifically, inside the finally block
(where controllerRef, inFlightRef, mountedRef, setIsLoading and setIsValidating
are updated) only clear refs and update state, and read-and-clear
pendingRef.current into a local boolean (e.g., const hadPending =
pendingRef.current; pendingRef.current = false) and also read
enabledRef.current, targetRef.current, and pollIntervalRef.current into locals;
then after the try/catch/finally (outside it) use those locals: if mountedRef
indicates mounted and hadPending then call runFetch(); else if enabled/local
target/pollInterval indicate polling should continue then call
schedulePoll(runFetch). This preserves behavior while removing unsafe returns in
the finally block.
In `@ui/src/pages/dag-runs/dag-run/index.tsx`:
- Around line 94-96: The guard comparing expectedDagRunId to
latestDetails.dagRunId incorrectly treats the sentinel 'latest' as a literal id;
update the logic around expectedDagRunId and dagRunDetails (used where
expectedDagRunId, subDAGRunId, dagRunId, latestDetails are referenced) so that
if expectedDagRunId === 'latest' it is treated as a wildcard (i.e., accept the
response) — for example, set dagRunDetails = latestDetails if latestDetails
exists and (expectedDagRunId === 'latest' || latestDetails.dagRunId ===
expectedDagRunId).
---
Nitpick comments:
In `@ui/src/features/dag-runs/hooks/dagRunDetailsRequest.ts`:
- Around line 1-4: This file is missing the required GPL v3 license header; add
the standard GPL v3 license header to the top of the file (above the imports) by
running the repository tool: execute make addlicense so the header is applied
automatically, ensuring it precedes the existing imports and type export
(references: the import lines for components and fetchJson and the exported type
DAGRunDetails).
In `@ui/src/features/dag-runs/hooks/useBoundedDAGRunDetails.ts`:
- Around line 188-196: In useBoundedDAGRunDetails, remove the redundant
mountedRef.current = true assignment inside the useEffect since mountedRef is
already initialized to true and refs persist; keep the cleanup callback that
sets mountedRef.current = false and calls clearPollTimer(),
abortActiveRequest(), and sets pendingRef.current = false, leaving the effect
dependencies ([abortActiveRequest, clearPollTimer]) unchanged.
- Around line 1-8: This file lacks the required GPL v3 license header; run the
project's license tooling to add it (e.g., execute the make addlicense target)
so the header is inserted at the top of the module containing
useBoundedDAGRunDetails (the file that imports fetchDAGRunDetails and
isAbortLikeError and declares useBoundedDAGRunDetails). Ensure the header
matches the project's GPL v3 template used by addlicense and is placed before
any imports or code.
In `@ui/src/lib/requestTimeout.ts`:
- Around line 1-6: This file is missing the required GPL v3 license header; add
the standard GPL v3 header to the top of the file (above the existing
declarations like READ_METHODS, DEFAULT_READ_TIMEOUT_MS,
DEFAULT_WRITE_TIMEOUT_MS, and LIVE_INVALIDATION_COOLDOWN_MS) by running the
repository's license tool (make addlicense) or inserting the project's standard
header so the file complies with the project's licensing guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd737f33-e229-43da-b2ee-5c56081a68fc
📒 Files selected for processing (18)
internal/persis/filedagrun/attempt.gointernal/persis/filedagrun/attempt_test.gointernal/persis/filedagrun/dagrun.gointernal/persis/filedagrun/dataroot.gointernal/persis/filedagrun/dataroot_test.gointernal/service/frontend/api/v1/dagruns.gointernal/service/frontend/api/v1/dagruns_internal_test.goui/src/App.tsxui/src/features/dag-runs/components/dag-run-details/DAGRunDetailsModal.tsxui/src/features/dag-runs/components/dag-run-details/DAGRunDetailsPanel.tsxui/src/features/dag-runs/components/dag-run-list/StepDetailsTooltip.tsxui/src/features/dag-runs/hooks/dagRunDetailsRequest.tsui/src/features/dag-runs/hooks/useBoundedDAGRunDetails.tsui/src/hooks/api.tsui/src/hooks/useAppLive.tsui/src/lib/fetchJson.tsui/src/lib/requestTimeout.tsui/src/pages/dag-runs/dag-run/index.tsx
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1827 +/- ##
==========================================
+ Coverage 69.21% 69.25% +0.03%
==========================================
Files 429 429
Lines 51586 51600 +14
==========================================
+ Hits 35706 35735 +29
+ Misses 12857 12845 -12
+ Partials 3023 3020 -3
... and 18 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
This was referenced Mar 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Refactor