seanmuth commented on code in PR #73693:
URL: https://github.com/apache/airflow/pull/73693#discussion_r4134496639
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/dags.py:
##########
@@ -274,9 +274,20 @@ def get_dag_details(
or 0
)
- # Add is_favorite and active_runs_count fields to the Dag model
+ # Count queued Dag runs: these are waiting for an active run to finish
before they can start.
+ queued_runs_count = (
+ session.scalar(
+ select(func.count())
+ .select_from(DagRun)
+ .where(DagRun.dag_id == dag_id, DagRun.state == DagRunState.QUEUED)
Review Comment:
Confirmed — added `DagRun.backfill_id.is_(None)` to both `active_runs_count`
and `queued_runs_count` (fixed the pre-existing `active_runs_count` bug too,
while I was right there), matching how
`get_queued_dag_runs_to_set_running`/`_start_queued_dagruns` scope concurrency
per `(dag_id, backfill_id)`. Also strengthened the count test to seed two
queued runs instead of one, and added a dedicated backfill-exclusion test —
pushed.
---
Drafted-by: Claude Sonnet 5; reviewed by @seanmuth before posting
##########
airflow-core/src/airflow/ui/src/pages/Dag/Header.tsx:
##########
@@ -113,11 +114,27 @@ export const Header = ({
},
...nextRunStat,
{
- label: translate("dagDetails.activeRuns"),
+ label:
+ (dag?.queued_runs_count ?? 0) > 0 ? (
Review Comment:
Good catch, done exactly as suggested — icon now only shows when
`active_runs_count >= max_active_runs && !is_paused`; the `(N queued)` text
stays visible whenever `queued_runs_count > 0` regardless. Added coverage for
the paused-at-capacity case and the below-capacity-with-queued case — pushed.
---
Drafted-by: Claude Sonnet 5; reviewed by @seanmuth before posting
##########
airflow-core/src/airflow/ui/src/pages/Dag/Header.tsx:
##########
@@ -113,11 +114,27 @@ export const Header = ({
},
...nextRunStat,
{
- label: translate("dagDetails.activeRuns"),
+ label:
+ (dag?.queued_runs_count ?? 0) > 0 ? (
+ <HStack gap={1}>
+ {translate("dagDetails.activeRuns")}
+ <Tooltip
content={translate("dagDetails.activeRunsExceedsMaxTooltip")}>
Review Comment:
Applied your suggestion directly — pushed.
---
Drafted-by: Claude Sonnet 5; reviewed by @seanmuth before posting
##########
airflow-core/src/airflow/ui/src/components/HeaderCard.tsx:
##########
@@ -77,8 +77,8 @@ export const HeaderCard = ({ actions, icon, state, stats,
subTitle, title, type
</Flex>
<HStack alignItems="flex-start" flexWrap="wrap" gap={6} my={3}>
- {stats.map((stat) => (
- <Box data-testid="stat" key={stat.key ?? stat.label}>
+ {stats.map((stat, index) => (
+ <Box data-testid="stat" key={stat.key ?? index}>
Review Comment:
Reverted to `stat.key ?? stat.label` (with a `typeof` narrowing check, since
`label`'s type still admits non-`Key` `ReactNode` values like `false`), and the
active-runs stat now passes `key: "activeRuns"` explicitly. `stats` is now a
discriminated union requiring `key` whenever `label` isn't a plain string —
pushed.
---
Drafted-by: Claude Sonnet 5; reviewed by @seanmuth before posting
##########
airflow-core/src/airflow/ui/src/pages/Dag/Header.test.tsx:
##########
@@ -93,6 +94,32 @@ describe("Header", () => {
expect(screen.getByText("2 of 2")).toBeInTheDocument();
});
+ it("does not show an info icon or queued count when nothing is queued", ()
=> {
+ render(
+ <Wrapper>
+ <Header dag={{ ...mockDag, active_runs_count: 1, max_active_runs: 2 }}
/>
+ </Wrapper>,
+ );
+
+
expect(screen.queryByTestId("active-runs-exceeds-max-info")).not.toBeInTheDocument();
+ expect(screen.getByText("1 of 2")).toBeInTheDocument();
+ });
+
+ it("shows an info icon and the queued count when runs are queued behind the
maximum", () => {
+ render(
+ <Wrapper>
+ <Header dag={{ ...mockDag, active_runs_count: 1, max_active_runs: 1,
queued_runs_count: 2 }} />
+ </Wrapper>,
+ );
+
+
expect(screen.getByTestId("active-runs-exceeds-max-info")).toBeInTheDocument();
+ expect(
+ screen.getByText(
+ i18n.t("common:dagDetails.activeRunsWithQueued", { activeRuns: 1,
maxActiveRuns: 1, queuedRuns: 2 }),
Review Comment:
Fixed — `Header.test.tsx` now loads the real `en/common` bundle (matching
`RenderedJsonField.test.tsx`) and asserts the literal pinned string, plus added
the at-capacity-nothing-queued case you flagged as missing — pushed.
---
Drafted-by: Claude Sonnet 5; reviewed by @seanmuth before posting
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_dags.py:
##########
@@ -1503,6 +1505,11 @@ def test_dag_details_includes_active_runs_count(self,
session, test_client):
assert isinstance(body["active_runs_count"], int)
assert body["active_runs_count"] == 1 # only running counts, queued
does not
+ # Verify queued_runs_count field is present and correct
+ assert "queued_runs_count" in body
+ assert isinstance(body["queued_runs_count"], int)
+ assert body["queued_runs_count"] == 1 # only queued counts,
running/success do not
Review Comment:
Fixed — now seeds two queued runs so `queued_runs_count` can't
coincidentally match `active_runs_count` if it queried the wrong state, dropped
the redundant `in`/`isinstance` asserts, and renamed the test to mention both
fields — pushed.
---
Drafted-by: Claude Sonnet 5; reviewed by @seanmuth before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
