Skip to content

feat(web): I/O token column for perf list and history record deletion - #1526

Merged
Yunnglin merged 6 commits into
mainfrom
fix/perf-web-io-tokens-and-delete
Jul 27, 2026
Merged

Yunnglin merged 6 commits into
mainfrom
fix/perf-web-io-tokens-and-delete

Conversation

@Yunnglin

Copy link
Copy Markdown
Collaborator

Summary

This PR delivers two Web UI feature requests for the history record lists:

Changes

Backend (evalscope/service)

  • GET /api/v1/perf/list now returns avg_input_tokens / avg_output_tokens per run, computed as a succeed-request-weighted average of the measured per-run summaries (backward compatible; fields are additive).
  • New DELETE /api/v1/perf/run (root_path + path):
    • path-traversal rejection via realpath prefix checks (reuses resolve_run_dir);
    • refuses non-perf-run directories and the outputs root itself;
    • running-task protection → 409 when the path belongs to an active task;
    • prunes now-empty parent directories (CLI <timestamp>/ husks) after deletion;
    • responds {"success": true, "path": "..."}.
  • New DELETE /api/v1/reports/report (root_path + report_name):
    • deletes per-model artefacts (reports/predictions/reviews/<model>) so runs holding multiple model reports stay intact;
    • removes the whole run directory once its last model report is gone;
    • same traversal / running-task protections as above.
  • active_task_ids() exposed from the service process registry.

Frontend (evalscope/web)

  • Performance list: new I→O tok column (desktop table + mobile meta line), rendered as e.g. 30000→100t; legitimate 0 values still render, only missing data shows —.
  • New shared ConfirmDialog component (dark-theme modal via portal, enumerates the affected records, danger-styled confirm button, initial focus on Cancel, focus trap, Esc/overlay cancel, busy lock) — replaces window.confirm.
  • Delete action wired into the shared SelectionTray on both the Evaluations and Performance pages; partial failures keep only the not-yet-deleted items selected and surface the error.
  • Delete wording unified under the generic reports.* i18n namespace ("Delete records" / 「删除记录」, en + zh) — no perf/eval-specific phrasing.
  • apiDeleteValidated client helper + zod schemas for the new endpoints.

Tests

  • Backend: tests/perf/test_perf_archive.py (13 cases) + new tests/report/test_report_delete.py (4 cases) — token fields, deletion success, parent-dir pruning, multi-model run preservation, traversal / invalid-name / running-task rejections. 17 passed.
  • Frontend: ConfirmDialog.test.tsx added; full suite 237 passed, tsc + vite build and eslint clean.
  • Manually verified end-to-end in the browser against evalscope service (column rendering en/zh, multi-select delete, confirm/cancel/Esc paths, on-disk cleanup).

Closes #1519
Closes #1521

Yunnglin added 2 commits July 27, 2026 14:24
…records

- Return succeed-weighted avg_input_tokens / avg_output_tokens in
  /api/v1/perf/list so the web UI can explain latency gaps (#1519)
- Add DELETE /api/v1/perf/run: path-traversal rejection via realpath,
  running-task protection (409), empty parent-dir pruning (#1521)
- Add DELETE /api/v1/reports/report: per-model artefact removal,
  whole run dir cleanup when the last model report is deleted (#1521)
- Expose active_task_ids() from the service process registry
- Cover token fields, deletion, traversal and 409 paths with unit tests
- Add I->O tok column to the performance run list (desktop table +
  mobile meta line), rendered as e.g. 30000->100t (#1519)
- Add a shared ConfirmDialog (dark-theme modal, affected-item list,
  focus management, busy lock) replacing window.confirm
- Wire delete actions into SelectionTray on both the Evaluations and
  Performance pages via new DELETE endpoints (#1521)
- Unify delete wording under the generic reports.* i18n namespace
- Guard delete confirm handlers against double-click re-entry
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@Yunnglin Yunnglin added the qoder-review Add to a PR to trigger Qoder code review label Jul 27, 2026

@qoderai qoderai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 Review Summary

This PR adds useful I/O token visibility for perf runs and a much more ergonomic way to delete historical perf and eval records from the web UI, with good attention to path-traversal guards, active-task checks, typed client contracts, and dedicated tests.

🛡️ Key Risks & Issues

  • Destructive use of client-controlled root_path: Both DELETE /api/v1/perf/run and DELETE /api/v1/reports/report rely on _root_path() and therefore trust the caller’s root_path query parameter as the base for destructive filesystem operations. While the implementations correctly confine deletions to descendants of that root and only touch paths that look like perf/report runs, any caller who can hit the service and choose root_path can target arbitrary directories that match the expected layout. This was low-risk when root_path only influenced reads; once it drives shutil.rmtree and full run-dir removal, it becomes much more important to constrain it to the canonical outputs root or validate that a provided value is under that configured root before allowing deletion.
  • Task-ID matching semantics for perf deletion: The running-task guard for perf uses a set of all path segments and intersects this with active_task_ids(). This works for the current <task_id>/perf layout, but it will also block deletion when a task ID coincides with any inner path segment (for example, a CLI run path that happens to contain the same token), leading to surprising “Task is still running” responses for directories that are not actually owned by a task. This is more of a behavioural edge case than a bug, but worth keeping in mind if layouts evolve.
  • Frontend deletion flows are untested: The batch deletion logic on PerfReportsPage and ReportsPage (sequential DELETEs, partial-failure handling, selection cleanup, reload triggers, and error messaging) is implemented carefully but currently only covered by manual verification. Given the potential impact of regressions in these flows on user trust and data cleanup, it would be valuable to add targeted tests that cover success and partial-failure scenarios, especially under filters/search.

🧪 Verification Advice

  • Exercise DELETE /api/v1/perf/run and /api/v1/reports/report against both service-style and CLI-style run trees, including edge cases: bad path / report_name, traversal attempts, and active-task IDs that should trigger 409. Confirm that the outputs root is never removed and that parent directories are pruned only when empty.
  • In a staging environment, verify how root_path is used end-to-end: check that the dashboard always sends the intended outputs root and that there is no way for non-admin clients to point it at arbitrary directories. If the service is exposed more broadly, consider tightening this contract before rollout.
  • For the web UI, test multi-select deletion of perf runs and reports with filters and search applied, including a simulated backend failure mid-batch (e.g., returning 400/409 for one item) to ensure the selection and error banner behave as expected.

💡 Thoughts & Suggestions

  • Consider constraining or removing root_path for destructive APIs: either always use the configured outputs root for DELETE operations or explicitly validate that a provided root_path is a descendant of that configured directory. This would keep the flexibility of root_path for read-only queries while reducing the blast radius for misconfigured or semi-trusted clients.
  • If the current task-ID layouts are expected to remain stable, the segment-based matching for running-task protection is acceptable; if you anticipate new layouts, it may be safer to narrow the check to known prefix positions. This would avoid future surprises where certain history records cannot be deleted due to incidental name collisions.
  • The new ConfirmDialog component and the I/O token column are solid additions with good UX and accessibility touches. Adding a few focused tests around keyboard focus-trapping and overlay-click behaviour would help ensure those qualities are preserved as the UI evolves.

🤖 Generated by Qoder • View workflow run

return jsonify({'error': str(e)}), 500


@bp_perf.route('/run', methods=['DELETE'])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new DELETE endpoints correctly guard against traversal and symlink escapes relative to the chosen root, but root_path itself is still entirely client-controlled and now drives destructive operations.

For perf runs, delete_perf_run resolves the base directory via _root_path() and passes it directly into perf_archive.delete_run(_root_path(), rel_path) (evalscope/service/blueprints/perf.py:386-409). delete_run then validates rel_path relative to that root and only removes directories recognized as perf runs (evalscope/service/perf_archive.py:587-603). This was safe while root_path only governed list/detail reads; with deletion wired to the same parameter, any caller that can hit the service can point root_path at an arbitrary directory tree containing perf-layout folders and delete them, as long as they match the expected run shape. That effectively turns root_path into a remote deletion capability for any perf-like tree on disk, which is a problem if the service is exposed beyond fully trusted operators.

The reports endpoint mirrors this pattern: delete_report also uses _root_path() to derive root_real and run_dir (evalscope/service/blueprints/reports.py:368-372) and then calls shutil.rmtree on per-model subtrees and potentially the entire run directory (lines 378-391). The path checks ensure nothing escapes root_real, but the caller still chooses root_real via root_path.

If these APIs are meant only for tightly controlled dashboards where root_path is constrained to the canonical outputs root, it would be safer to either (a) ignore root_path for DELETE and always use the configured outputs root, or (b) validate that a provided root_path is a descendant of the configured outputs directory before allowing deletion. Otherwise, a misconfigured client or semi-trusted integration could use these endpoints to delete data outside the intended evalscope outputs area while still passing all internal safety checks.


🤖 Generated by Qoder • Fix in Qoder

@Yunnglin
Yunnglin merged commit b5da52c into main Jul 27, 2026
4 checks passed
@Yunnglin
Yunnglin deleted the fix/perf-web-io-tokens-and-delete branch September 10, 2026 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

qoder-review Add to a PR to trigger Qoder code review

Projects

None yet

1 participant