fix(validation): process all dashboards in a run, not just the first one
Root cause of stuck 'running' runs: the plugin's execute() method
only processed dashboard_ids[0] and returned. The remaining N-1
dashboards were never processed, and _update_run_status was called
after each single dashboard without checking dashboard_count.
Fixes:
- execute() now loops over ALL dashboard_ids and processes each
via _execute_path_a or _execute_path_b
- _update_run_status is called once AFTER the loop finishes
- _update_run_status now checks that len(records) >= dashboard_count
before marking the run as finished (prevents premature completion)
- Per-dashboard _update_run_status calls removed from _execute_path_a
and _execute_path_b (the parent loop owns it now)
- Test updated for new batch return format {dashboards: [...], total: N}
This commit is contained in:
@@ -104,27 +104,31 @@ def _ensure_json_prompt(prompt: str | None) -> str:
|
||||
|
||||
|
||||
# #region _update_run_status [TYPE Function]
|
||||
# @BRIEF Update ValidationRun status based on its records' aggregate status.
|
||||
# @PRE db session is active, run_id may be None (v1 records or direct trigger).
|
||||
# @POST If run_id is set, ValidationRun.status is updated to reflect the worst record status.
|
||||
# @BRIEF Aggregate ValidationRecord statuses into the ValidationRun.
|
||||
# Only marks run as finished when ALL dashboards have a record.
|
||||
# @PRE db session is active, run_id may be None.
|
||||
# @POST If run_id is set and all expected dashboards are recorded,
|
||||
# ValidationRun.status is updated to the worst record status.
|
||||
def _update_run_status(db, run_id: str | None) -> None:
|
||||
"""Aggregate ValidationRecord statuses into the ValidationRun."""
|
||||
if not run_id:
|
||||
return
|
||||
from ..models.llm import ValidationRecord as VR, ValidationRun as VRun
|
||||
try:
|
||||
records = db.query(VR).filter(VR.run_id == run_id).all()
|
||||
if not records:
|
||||
run = db.query(VRun).filter(VRun.id == run_id).first()
|
||||
if not records or not run:
|
||||
return
|
||||
# Only mark complete when ALL expected dashboards have a record
|
||||
total_expected = run.dashboard_count or 0
|
||||
if total_expected > 0 and len(records) < total_expected:
|
||||
return
|
||||
# Determine worst status: FAIL > WARN > UNKNOWN > PASS
|
||||
priority = {"FAIL": 0, "WARN": 1, "UNKNOWN": 2, "PASS": 3}
|
||||
worst = min(records, key=lambda r: priority.get(r.status, 99))
|
||||
run = db.query(VRun).filter(VRun.id == run_id).first()
|
||||
if run:
|
||||
run.status = worst.status
|
||||
from datetime import datetime, timezone
|
||||
run.finished_at = datetime.now(timezone.utc)
|
||||
db.commit()
|
||||
run.status = worst.status
|
||||
from datetime import datetime, timezone
|
||||
run.finished_at = datetime.now(timezone.utc)
|
||||
db.commit()
|
||||
except Exception:
|
||||
pass
|
||||
# #endregion _update_run_status
|
||||
@@ -235,59 +239,72 @@ class DashboardValidationPlugin(PluginBase):
|
||||
"Please open LLM provider settings and save a real API key (not masked placeholder)."
|
||||
)
|
||||
|
||||
# Resolve dashboard_id from v2 list or v1 single string
|
||||
dashboard_ids = params.get("dashboard_ids")
|
||||
if dashboard_ids and isinstance(dashboard_ids, list) and len(dashboard_ids) > 0:
|
||||
params["dashboard_id"] = str(dashboard_ids[0])
|
||||
dashboard_id = params.get("dashboard_id")
|
||||
if not dashboard_id:
|
||||
# Resolve dashboard_ids from v2 list or v1 single string
|
||||
all_dashboard_ids: list[str] = []
|
||||
raw_ids = params.get("dashboard_ids")
|
||||
if raw_ids and isinstance(raw_ids, list) and len(raw_ids) > 0:
|
||||
all_dashboard_ids = [str(did) for did in raw_ids]
|
||||
single_id = params.get("dashboard_id")
|
||||
if single_id and single_id not in all_dashboard_ids:
|
||||
all_dashboard_ids.append(str(single_id))
|
||||
if not all_dashboard_ids:
|
||||
raise ValueError("No dashboard_id provided in params (dashboard_ids list is empty or missing)")
|
||||
|
||||
# Re-parse URL sources per FR-057
|
||||
dashboard_url = params.get("dashboard_url")
|
||||
if dashboard_url:
|
||||
try:
|
||||
from ...core.utils.superset_context_extractor import SupersetContextExtractor
|
||||
|
||||
client = SupersetClient(env)
|
||||
extractor = SupersetContextExtractor(environment=env, client=client)
|
||||
parsed = extractor.parse_superset_link(dashboard_url)
|
||||
|
||||
if parsed.dashboard_id:
|
||||
params["dashboard_id"] = str(parsed.dashboard_id)
|
||||
|
||||
# Store parsed context for tab/filter navigation
|
||||
params["parsed_context"] = {
|
||||
"dashboard_id": parsed.dashboard_id,
|
||||
"native_filters": parsed.imported_filters if hasattr(parsed, "imported_filters") else None,
|
||||
"active_tabs": [],
|
||||
"anchor": None,
|
||||
}
|
||||
|
||||
log.info(f"[URL re-parse] Resolved dashboard_id={parsed.dashboard_id} from URL")
|
||||
|
||||
except Exception as e:
|
||||
log.warning(f"[URL re-parse] Failed to parse URL: {e}")
|
||||
# Mark source as failed — task will skip this dashboard
|
||||
params["source_invalid"] = True
|
||||
params["source_error"] = str(e)
|
||||
|
||||
screenshot_enabled = params.get("screenshot_enabled", True)
|
||||
if screenshot_enabled and not bool(db_provider.is_multimodal):
|
||||
raise ValueError(
|
||||
"Dashboard validation with screenshot_enabled=True requires "
|
||||
"a multimodal model (image input support)."
|
||||
)
|
||||
|
||||
if screenshot_enabled:
|
||||
# Path A requires a multimodal provider for image input
|
||||
if not bool(db_provider.is_multimodal):
|
||||
raise ValueError(
|
||||
"Dashboard validation with screenshot_enabled=True requires "
|
||||
"a multimodal model (image input support)."
|
||||
# Process each dashboard sequentially, collecting results
|
||||
results: list[dict[str, Any]] = []
|
||||
for idx, dashboard_id in enumerate(all_dashboard_ids):
|
||||
log.info(f"Processing dashboard {idx + 1}/{len(all_dashboard_ids)}: {dashboard_id}")
|
||||
params["dashboard_id"] = dashboard_id
|
||||
|
||||
# Re-parse URL sources per FR-057
|
||||
dashboard_url = params.get("dashboard_url")
|
||||
if dashboard_url:
|
||||
try:
|
||||
from ...core.utils.superset_context_extractor import SupersetContextExtractor
|
||||
|
||||
client = SupersetClient(env)
|
||||
extractor = SupersetContextExtractor(environment=env, client=client)
|
||||
parsed = extractor.parse_superset_link(dashboard_url)
|
||||
|
||||
if parsed.dashboard_id:
|
||||
params["dashboard_id"] = str(parsed.dashboard_id)
|
||||
|
||||
# Store parsed context for tab/filter navigation
|
||||
params["parsed_context"] = {
|
||||
"dashboard_id": parsed.dashboard_id,
|
||||
"native_filters": parsed.imported_filters if hasattr(parsed, "imported_filters") else None,
|
||||
"active_tabs": [],
|
||||
"anchor": None,
|
||||
}
|
||||
|
||||
log.info(f"[URL re-parse] Resolved dashboard_id={parsed.dashboard_id} from URL")
|
||||
|
||||
except Exception as e:
|
||||
log.warning(f"[URL re-parse] Failed to parse URL: {e}")
|
||||
# Mark source as failed — task will skip this dashboard
|
||||
params["source_invalid"] = True
|
||||
params["source_error"] = str(e)
|
||||
|
||||
if screenshot_enabled:
|
||||
result = await self._execute_path_a(
|
||||
params, context, db, db_provider, api_key, env, config_mgr, log
|
||||
)
|
||||
return await self._execute_path_a(
|
||||
params, context, db, db_provider, api_key, env, config_mgr, log
|
||||
)
|
||||
else:
|
||||
return await self._execute_path_b(
|
||||
params, context, db, db_provider, api_key, env, config_mgr, log
|
||||
)
|
||||
else:
|
||||
result = await self._execute_path_b(
|
||||
params, context, db, db_provider, api_key, env, config_mgr, log
|
||||
)
|
||||
results.append(result)
|
||||
|
||||
# After ALL dashboards processed, update run status to reflect completion
|
||||
_update_run_status(db, params.get("run_id"))
|
||||
return {"dashboards": results, "total": len(results)}
|
||||
|
||||
finally:
|
||||
db.close()
|
||||
@@ -466,9 +483,6 @@ class DashboardValidationPlugin(PluginBase):
|
||||
db.add(db_record)
|
||||
db.commit()
|
||||
|
||||
# Update ValidationRun status after record is persisted
|
||||
_update_run_status(db, params.get("run_id"))
|
||||
|
||||
# 7. Notification on failure (US1 / FR-015)
|
||||
try:
|
||||
policy_id = params.get("policy_id")
|
||||
@@ -679,9 +693,6 @@ class DashboardValidationPlugin(PluginBase):
|
||||
db.add(db_record)
|
||||
db.commit()
|
||||
|
||||
# Update ValidationRun status after record is persisted
|
||||
_update_run_status(db, params.get("run_id"))
|
||||
|
||||
# 8. Notification on failure (US1 / FR-015)
|
||||
try:
|
||||
policy_id = params.get("policy_id")
|
||||
|
||||
@@ -218,7 +218,9 @@ async def test_dashboard_validation_plugin_persists_task_and_environment_ids(
|
||||
context=context,
|
||||
)
|
||||
|
||||
assert result["environment_id"] == "env-42"
|
||||
# Plugin now returns a batch result with all dashboards
|
||||
assert result["total"] == 1
|
||||
assert result["dashboards"][0]["environment_id"] == "env-42"
|
||||
assert fake_db.committed is True
|
||||
assert fake_db.closed is True
|
||||
assert fake_db.added is not None
|
||||
|
||||
Reference in New Issue
Block a user