Consolidate measurement artifact contracts

docs/guides/add-app.md

@@ -487,7 +487,8 @@ tar -czf ../results/padata0.tgz ./pa
 
 Fugaku 系アプリでは、アプリ側が profiler tool を内部で選び、Benchkit 共通の `bk_profiler` helper に渡す形が扱いやすいです。
 `bk_profiler` は profiler ごとの raw data / postprocess report をまとめて `results/padata*.tgz` に保存し、archive 内の `bk_profiler_artifact/meta.json` に metadata を入れます。Benchkit や推定 package はこの `meta.json` を見て、tool、level、report kind を機械的に判断できます。
-`timing_observations` が `results/*.json` を参照する場合も、Result 送信時に同じ Measurement Artifacts として保存されます。
+
+アプリが独自の詳細 timer table を持つ場合は、まず小さな `results/*.json` として保存し、`bk_record_timing_observation` で登録してください。この JSON は Result 送信時に Measurement Artifacts として保存されます。`timing_observations` は未レビューの観測値を残すための任意機能であり、`SECTION:` / `OVERLAP:` や `fom_breakdown` へ昇格するには、timer ID、inclusive / exclusive の扱い、overlap window の意味を別途レビューします。
 
 `fapp` では共通 level として次を扱います。
 

docs/guides/developer-reference.md

@@ -283,5 +283,6 @@ Treat missing `source_info`, `fom_breakdown`, or artifact references as follow-u
 Detailed timing artifacts may be recorded through `timing_observations` before
 they are promoted to `fom_breakdown`; do not treat every detailed timer or
 profiler region as an additive estimation section without an app-specific
-mapping review. Referenced `results/*.json` timing files and profiler archives
-are uploaded as Measurement Artifacts by the result sender.
+mapping review. Apps should use `bk_record_timing_observation` for this
+handoff. Referenced `results/*.json` timing files and profiler archives are
+uploaded as Measurement Artifacts by the result sender.

result_server/routes/api.py

@@ -16,14 +16,16 @@
 from utils.auth import verify_ingest_key, verify_trusted_proxy_auth
 from utils.audit_logging import audit_event
 from utils.environment_snapshots import index_environment_snapshot
+from utils.measurement_artifacts import (
+    is_profile_archive_basename,
+    normalize_measurement_artifact_basename,
+    stored_measurement_artifact_filename,
+)
 from utils.rate_limit import rate_limited
 from utils.result_metadata_index import index_result_metadata
 
 api_bp = Blueprint("api", __name__)
 _TIMESTAMP_RE = re.compile(r"^\d{8}_\d{6}$")
-_MEASUREMENT_ARTIFACT_BASENAME_RE = re.compile(
-    r"[A-Za-z0-9][A-Za-z0-9_.-]{0,127}\.(?:tgz|tar\.gz|json)"
-)
 DEFAULT_MAX_ARCHIVE_MEMBER_SIZE = 1024 * 1024 * 1024
 DEFAULT_MAX_ARCHIVE_TOTAL_EXTRACTED_SIZE = 1024 * 1024 * 1024
 DEFAULT_MAX_ARCHIVE_MEMBER_COUNT = 4096
@@ -214,41 +216,6 @@ def _safe_basename(name):
     return name
 
 
-def _normalize_measurement_artifact_basename(
-    value,
-    *,
-    error_message="Invalid measurement artifact path",
-):
-    """Return a filename-safe results/ artifact basename, or None."""
-    if value is None:
-        return None
-
-    artifact_path = str(value).strip()
-    if artifact_path == "":
-        return None
-    if (
-        os.path.isabs(artifact_path)
-        or "\\" in artifact_path
-        or artifact_path.startswith("../")
-        or "/../" in artifact_path
-        or artifact_path.endswith("/..")
-    ):
-        abort(400, description=error_message)
-    if not artifact_path.startswith("results/"):
-        abort(400, description=error_message)
-
-    basename = os.path.basename(artifact_path)
-    if not _MEASUREMENT_ARTIFACT_BASENAME_RE.fullmatch(basename):
-        abort(400, description=error_message)
-    return basename
-
-
-def _is_profile_archive_basename(basename):
-    return isinstance(basename, str) and (
-        basename.endswith(".tgz") or basename.endswith(".tar.gz")
-    )
-
-
 def _copy_uploaded_file(uploaded_file, save_path):
     """Write an uploaded file atomically."""
     tmp_path = save_path + ".tmp"
@@ -259,21 +226,6 @@ def _copy_uploaded_file(uploaded_file, save_path):
     os.rename(tmp_path, save_path)
 
 
-def _measurement_artifact_filename(timestamp, uuid_str, artifact_basename):
-    if artifact_basename is None:
-        return _safe_basename(f"padata_{timestamp}_{uuid_str}.tgz")
-    if _is_profile_archive_basename(artifact_basename):
-        artifact_slug = (
-            artifact_basename[:-7]
-            if artifact_basename.endswith(".tar.gz")
-            else artifact_basename[:-4]
-        )
-        return _safe_basename(f"padata_{timestamp}_{uuid_str}_{artifact_slug}.tgz")
-    return _safe_basename(
-        f"measurement_artifact_{timestamp}_{uuid_str}_{artifact_basename}"
-    )
-
-
 def _load_json_by_uuid(directory, field_path, uuid_value):
     """Return the first JSON payload whose target field matches the UUID."""
     json_files = sorted(
@@ -493,16 +445,21 @@ def ingest_measurement_artifact():
         "RECEIVED_MEASUREMENT_ARTIFACTS_DIR",
         current_app.config.get("RECEIVED_PADATA_DIR", current_app.config["RECEIVED_DIR"]),
     )
-    artifact_basename = _normalize_measurement_artifact_basename(
-        request.form.get("artifact_path")
-    )
-    if artifact_basename is None and not _is_profile_archive_basename(
+    try:
+        artifact_basename = normalize_measurement_artifact_basename(
+            request.form.get("artifact_path")
+        )
+    except ValueError:
+        abort(400, description="Invalid measurement artifact path")
+    if artifact_basename is None and not is_profile_archive_basename(
         uploaded_file.filename or ""
     ):
         abort(400, description="Missing measurement artifact path")
 
     if artifact_basename:
-        filename = _measurement_artifact_filename(timestamp, uuid_str, artifact_basename)
+        filename = _safe_basename(
+            stored_measurement_artifact_filename(timestamp, uuid_str, artifact_basename)
+        )
         matched_files = (
             [filename] if os.path.exists(os.path.join(received_dir, filename)) else []
         )
@@ -520,7 +477,9 @@ def ingest_measurement_artifact():
         save_path = old_file_path
     else:
         if not artifact_basename:
-            filename = _measurement_artifact_filename(timestamp, uuid_str, None)
+            filename = _safe_basename(
+                stored_measurement_artifact_filename(timestamp, uuid_str, None)
+            )
         save_path = os.path.join(received_dir, filename)
 
     _copy_uploaded_file(uploaded_file, save_path)

result_server/routes/results_detail_routes.py

@@ -1,6 +1,5 @@
 import json
 import os
-import re
 
 from flask import abort, current_app, render_template, request, url_for
 from werkzeug.exceptions import Forbidden, NotFound
@@ -31,6 +30,10 @@
     serve_permitted_result_file,
     serve_public_padata_file,
 )
+from utils.measurement_artifacts import (
+    is_measurement_artifact_filename,
+    stored_measurement_artifact_filename_from_path,
+)
 from utils.result_records import (
     format_numeric_value,
     format_result_timestamp,
@@ -39,20 +42,6 @@
 from utils.trigger_display import load_trigger_run_lookup, summarize_execution_trigger
 
 
-PADATA_ARTIFACT_BASENAME_RE = re.compile(
-    r"[A-Za-z0-9][A-Za-z0-9_.-]{0,127}\.(?:tgz|tar\.gz)"
-)
-MEASUREMENT_ARTIFACT_BASENAME_RE = re.compile(
-    r"[A-Za-z0-9][A-Za-z0-9_.-]{0,127}\.(?:tgz|tar\.gz|json)"
-)
-MEASUREMENT_ARTIFACT_FILENAME_RE = re.compile(
-    r"^measurement_artifact_\d{8}_\d{6}_"
-    r"[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}_"
-    r"[A-Za-z0-9][A-Za-z0-9_.-]{0,127}\.(?:tgz|tar\.gz|json)$",
-    re.IGNORECASE,
-)
-
-
 def register_results_detail_routes(results_bp):
     def public_surface():
         return current_app.config.get("PUBLIC_PORTAL_MODE", False)
@@ -104,7 +93,7 @@ def result_detail(filename):
             if is_public_surface
             else [
                 name for name in os.listdir(artifact_dir)
-                if _is_measurement_artifact_filename(name)
+                if is_measurement_artifact_filename(name)
             ]
         )
         detail_context = build_result_detail_context(
@@ -309,7 +298,7 @@ def show_result(filename):
                 )
             abort(404)
 
-        if _is_measurement_artifact_filename(filename):
+        if is_measurement_artifact_filename(filename):
             return serve_permitted_result_file(
                 filename,
                 current_app.config["RECEIVED_DIR"],
@@ -353,10 +342,13 @@ def _list_result_padata_filenames(result, padata_dir):
     filenames = []
     seen = set()
     for artifact_path in _iter_result_padata_artifact_paths(result):
-        artifact_slug = _padata_artifact_slug(artifact_path)
-        if not artifact_slug:
+        filename = stored_measurement_artifact_filename_from_path(
+            timestamp,
+            result_uuid,
+            artifact_path,
+        )
+        if not filename or not filename.endswith(".tgz"):
             continue
-        filename = f"padata_{timestamp}_{result_uuid}_{artifact_slug}.tgz"
         if filename in seen:
             continue
         seen.add(filename)
@@ -381,7 +373,11 @@ def _list_result_measurement_artifact_filenames(result, artifact_dir, *, include
         return filenames
 
     for artifact_path in _iter_result_timing_artifact_paths(result):
-        filename = _measurement_artifact_filename(timestamp, result_uuid, artifact_path)
+        filename = stored_measurement_artifact_filename_from_path(
+            timestamp,
+            result_uuid,
+            artifact_path,
+        )
         if not filename or filename in seen:
             continue
         seen.add(filename)
@@ -421,36 +417,5 @@ def _iter_result_timing_artifact_paths(result):
             yield path
 
 
-def _padata_artifact_slug(artifact_path):
-    if not isinstance(artifact_path, str) or not artifact_path.startswith("results/"):
-        return ""
-    basename = os.path.basename(artifact_path)
-    if not PADATA_ARTIFACT_BASENAME_RE.fullmatch(basename):
-        return ""
-    return basename[:-7] if basename.endswith(".tar.gz") else basename[:-4]
-
-
-def _measurement_artifact_filename(timestamp, result_uuid, artifact_path):
-    basename = _measurement_artifact_basename(artifact_path)
-    if not basename:
-        return ""
-    return f"measurement_artifact_{timestamp}_{result_uuid}_{basename}"
-
-
-def _measurement_artifact_basename(artifact_path):
-    if not isinstance(artifact_path, str) or not artifact_path.startswith("results/"):
-        return ""
-    basename = os.path.basename(artifact_path)
-    if not MEASUREMENT_ARTIFACT_BASENAME_RE.fullmatch(basename):
-        return ""
-    return basename
-
-
-def _is_measurement_artifact_filename(filename):
-    return filename.endswith(".tgz") or bool(
-        MEASUREMENT_ARTIFACT_FILENAME_RE.fullmatch(filename)
-    )
-
-
 def _clean_result_value(value):
     return str(value or "").strip()

result_server/tests/test_api_routes.py

@@ -503,6 +503,20 @@ def test_rejects_invalid_measurement_artifact_path(self, client, artifact_path):
         )
         assert resp.status_code == 400
 
+    def test_requires_artifact_path_for_non_archive_upload(self, client):
+        data = {
+            "id": "12345678-1234-1234-1234-123456789abc",
+            "timestamp": "20250101_120000",
+            "file": (io.BytesIO(b'{"timers": []}'), "qws_timing_CASE0.json"),
+        }
+        resp = client.post(
+            "/api/ingest/measurement-artifact",
+            data=data,
+            headers={"X-API-Key": API_KEY},
+            content_type="multipart/form-data",
+        )
+        assert resp.status_code == 400
+
     def test_missing_api_key_returns_401(self, client):
         resp = client.post(
             "/api/ingest/measurement-artifact",

result_server/tests/test_measurement_artifacts.py

@@ -0,0 +1,107 @@
+import os
+import sys
+
+import pytest
+
+sys.path.insert(0, os.path.join(os.path.dirname(__file__), ".."))
+
+from utils.measurement_artifacts import (
+    PUBLIC_PADATA_FILENAME_RE,
+    is_measurement_artifact_filename,
+    measurement_artifact_basename_from_path,
+    normalize_measurement_artifact_basename,
+    profile_archive_filename_candidates,
+    stored_measurement_artifact_filename,
+    stored_measurement_artifact_filename_from_path,
+)
+
+
+UUID = "12345678-1234-1234-1234-123456789abc"
+TIMESTAMP = "20250101_120000"
+
+
+def test_normalizes_safe_results_artifact_paths():
+    assert normalize_measurement_artifact_basename("results/qws_timing_CASE0.json") == (
+        "qws_timing_CASE0.json"
+    )
+    assert normalize_measurement_artifact_basename("results/padata_pairlist.tgz") == (
+        "padata_pairlist.tgz"
+    )
+    assert normalize_measurement_artifact_basename("") is None
+    assert normalize_measurement_artifact_basename(None) is None
+
+
+@pytest.mark.parametrize(
+    "artifact_path",
+    [
+        "../qws_timing.json",
+        "results/../qws_timing.json",
+        "/tmp/qws_timing.json",
+        "results/bad name.json",
+        "artifacts/qws_timing.json",
+        "results/qws_timing.txt",
+    ],
+)
+def test_rejects_unsafe_or_unsupported_artifact_paths(artifact_path):
+    with pytest.raises(ValueError):
+        normalize_measurement_artifact_basename(artifact_path)
+    assert measurement_artifact_basename_from_path(artifact_path) == ""
+
+
+def test_builds_canonical_stored_names_for_legacy_profiles_and_timing_json():
+    assert stored_measurement_artifact_filename(TIMESTAMP, UUID, None) == (
+        f"padata_{TIMESTAMP}_{UUID}.tgz"
+    )
+    assert stored_measurement_artifact_filename_from_path(
+        TIMESTAMP,
+        UUID,
+        "results/padata_pairlist.tgz",
+    ) == f"padata_{TIMESTAMP}_{UUID}_padata_pairlist.tgz"
+    assert stored_measurement_artifact_filename_from_path(
+        TIMESTAMP,
+        UUID,
+        "results/qws_timing_CASE0.json",
+    ) == f"measurement_artifact_{TIMESTAMP}_{UUID}_qws_timing_CASE0.json"
+
+
+def test_profile_archive_candidates_keep_legacy_and_generic_names():
+    assert profile_archive_filename_candidates(
+        TIMESTAMP,
+        UUID,
+        "results/padata_pairlist.tar.gz",
+    ) == [
+        f"padata_{TIMESTAMP}_{UUID}_padata_pairlist.tgz",
+        f"measurement_artifact_{TIMESTAMP}_{UUID}_padata_pairlist.tar.gz",
+    ]
+
+
+@pytest.mark.parametrize(
+    "filename",
+    [
+        f"padata_{TIMESTAMP}_{UUID}.tgz",
+        f"padata_{TIMESTAMP}_{UUID}_padata_pairlist.tgz",
+        f"measurement_artifact_{TIMESTAMP}_{UUID}_qws_timing_CASE0.json",
+    ],
+)
+def test_recognizes_served_measurement_artifact_filenames(filename):
+    assert is_measurement_artifact_filename(filename)
+
+
+def test_public_padata_filename_pattern_matches_full_uuid():
+    assert PUBLIC_PADATA_FILENAME_RE.fullmatch(f"padata_{TIMESTAMP}_{UUID}.tgz")
+    assert PUBLIC_PADATA_FILENAME_RE.fullmatch(
+        f"padata_{TIMESTAMP}_{UUID}_padata_pairlist.tgz"
+    )
+
+
+@pytest.mark.parametrize(
+    "filename",
+    [
+        "measurement_artifact_20250101_120000_qws_timing_CASE0.json",
+        f"measurement_artifact_{TIMESTAMP}_{UUID}_bad name.json",
+        "nested/debug_bundle.tgz",
+        "debug_bundle.json",
+    ],
+)
+def test_rejects_unserved_measurement_artifact_filenames(filename):
+    assert not is_measurement_artifact_filename(filename)