From f875ebb22af3b4416a44a0719d38a12bfb742096 Mon Sep 17 00:00:00 2001 From: Eomdahyeon <213566566+Eomdahyeon@users.noreply.github.com> Date: Tue, 6 Oct 2026 08:02:57 +0900 Subject: [PATCH] fix(api): refuse an interrupted run's id to its submitter too A run id is one attempt (kpubdata#812). #1027 let the submitter of a run a restart interrupted build under the same id again; the second attempt's events followed the first attempt's run_failed and each ending read as the other's state. Refuse the id as a completed run's id is refused - 409 on POST /builds, 400 on POST /build, whose 409 is a build response - and tell the client to submit under a new run_id. Refs #1042 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + contract/builder-api.yaml | 15 +++-- contract/fixtures/responses.json | 2 +- src/kpubdata_builder/service/app.py | 5 +- .../service/build_runs_api.py | 6 +- .../service/routes/_guards.py | 27 ++++++-- src/kpubdata_builder/service/routes/core.py | 3 +- tests/unit/test_interrupted_run_status.py | 63 ++++++++++++++++--- 8 files changed, 97 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2eaf336b..0695443c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixed +- A `run_id` is one attempt (#1042, decided in kpubdata#812, API contract 1.83.0). #1027 let the submitter of a run a restart interrupted build under the same id again; the second attempt's events then followed the first attempt's `run_failed`, and each ending read as the other's state. The id of a run that ended without a manifest is now refused to its submitter too — 409 on `POST /builds`, as a completed run's id is, and 400 on `POST /build`, whose 409 is a build response — and the interrupted run's message says to submit again under a new `run_id`. **Behaviour change** for a client that resubmitted an interrupted run under its old id. Pointing the new run at the earlier one (`retry_of`) is not in this change. - A build waiting for a slot no longer holds a request thread without a bound, and an async one no longer reads `running` (#1040, API contract 1.82.0, additive). Since #1028 every build takes one of `KPUBDATA_BUILDER_MAX_BUILDS` slots. A synchronous `POST /build` now waits for one at most `KPUBDATA_BUILDER_BUILD_WAIT_SECONDS` (default 30) and then answers 429 `build_queue_full` — the code `POST /builds` gives for a full queue — without having fetched anything, so a burst cannot park the request pool behind the build limit. An async worker takes its slot before the job is marked running: a job waiting for one stays `queued`, and one cancelled while waiting ends `cancelled` once without having started. - The number of builds running at once is its own limit, and it holds for both ways a build starts (#1028). `KPUBDATA_BUILDER_MAX_WORKERS` sized the request threads and the async build workers together, so the production compose's `2` let two synchronous `POST /build` run on the request threads on top of two async jobs — four builds against a memory budget computed for two — and left every other request two threads. `KPUBDATA_BUILDER_MAX_BUILDS` (`serve --max-builds`) now bounds builds on both paths together: at the limit a synchronous build waits and an async job stays queued. `KPUBDATA_BUILDER_MAX_PREVIEWS` (`serve --max-previews`, unset = no limit, as before) bounds previews, which the budget counted as one but nothing limited. Unset, `MAX_BUILDS` follows `MAX_WORKERS`, so a deployment that only set that keeps its async worker count — and that number is now the total. **Production compose:** `MAX_BUILDS` defaults to 2, `MAX_PREVIEWS` to 1, and `MAX_WORKERS` goes back to 10; the budget check multiplies by the two limits the service enforces. - `POST /builds` no longer returns another owner's job (#991). A `run_id` that was already queued, running, or had failed before writing a manifest was handed back with 200 to whoever named it — `created_by`, the build's response body and its error — because the route had only the 409 for a completed run. It now applies the rule `POST /build` has had since #635: the owner still gets the existing job, anyone else gets 403 `forbidden: not run owner`. diff --git a/contract/builder-api.yaml b/contract/builder-api.yaml index 25e8754a..f18de60b 100644 --- a/contract/builder-api.yaml +++ b/contract/builder-api.yaml @@ -1,7 +1,7 @@ openapi: 3.1.0 info: title: KPubData Builder Service API - version: "1.82.0" + version: "1.83.0" description: >- KPubData Builder(패키지 `kpubdata-builder`) 서비스 계약. BuildSpec 검증, preview, build 실행, artifact 조회, 계약 버전 조회를 제공한다. CLI와 HTTP service mode(#36)가 동일한 도메인 계약을 @@ -20,6 +20,7 @@ info: 추가한다(#504, additive). v1.8.0은 stable principal별 encrypted Provider credential CRUD와 Provider status/test API를 추가한다(#492, additive). v1.9.0은 `/query` 응답에 child startup과 Polars engine 실행 시간을 추가한다(#523, additive). + v1.83.0 makes a `run_id` one attempt (#1042, kpubdata#812): the id of a run that ended without a manifest — one a restart interrupted — is refused to its own submitter too — with 409 on `POST /builds`, as a completed run's id is, and with 400 on `POST /build`, whose 409 is a build response with a different body. v1.82.0 bounds how long a synchronous `POST /build` waits for a build slot (#1040, additive): past `KPUBDATA_BUILDER_BUILD_WAIT_SECONDS` it answers 429 `build_queue_full`, the code `POST /builds` already gives for a full queue, without having fetched anything. v1.81.0 declares the publish recovery routes the service has answered since v1.19.0 and v1.20.0 (#994, additive): `GET` and `DELETE /builds/{run_id}/publish/receipt`, `POST /builds/{run_id}/publish/reconcile` and `GET /builds/{run_id}/publish/audit` were described only in this text, so a client had no schema to check them against. Nothing on the wire changed. v1.80.0 declares what the service already sends (#994, description only — the wire is unchanged): the `X-Provider-Key` request header as a parameter of the five operations that call a provider (`previewBuild`, `createBuild`, `submitBuild`, `getProviderStatus`, `testProviderConnection`), the 429 `auth_throttled` and the overload 503 as shared responses any operation can answer (`components.responses.AuthThrottled`, `ServerOverloaded`), and the `X-Request-ID` response header (`components.headers.RequestId`). @@ -857,7 +858,10 @@ paths: manifest: build/air-quality-20260815/manifest.json api_version: "1.8.0" "400": - description: 잘못된 요청(spec 누락/검증 실패, run_id 타입/경로 안전성 오류 등) + description: >- + 잘못된 요청(spec 누락/검증 실패, run_id 타입/경로 안전성 오류 등). 매니페스트 없이 + 끝난 run(재시작으로 중단된 run)의 `run_id` 를 다시 쓴 경우도 여기다(#1042) — + `run_id` 는 한 번의 시도이고, 재시도는 새 `run_id` 로 보낸다. content: application/json: schema: @@ -1478,7 +1482,9 @@ paths: "401": $ref: "#/components/responses/Unauthorized" "409": - description: run_id가 이미 완료된 run + description: >- + run_id가 이미 완료된 run, 또는 매니페스트 없이 끝난 run(재시작으로 중단된 + run, #1042). `run_id` 는 한 번의 시도다 — 재시도는 새 `run_id` 로 보낸다. content: application/json: schema: @@ -7938,7 +7944,8 @@ components: A stable reason for a failure the service itself caused (#996), when there is one. `credentials_required`: the server restarted while the job was queued or running; its provider keys were held only in memory (#683), so - it cannot go on — submit it again. + it cannot go on — submit it again under a new `run_id`. The interrupted + run's id is not reused: a `run_id` is one attempt (#1042). BuildsResponse: type: object diff --git a/contract/fixtures/responses.json b/contract/fixtures/responses.json index 2e9738fe..8d81fbc8 100644 --- a/contract/fixtures/responses.json +++ b/contract/fixtures/responses.json @@ -1,6 +1,6 @@ { "fixture_format": 1, - "contract_version": "1.82.0", + "contract_version": "1.83.0", "probe_field": "future_optional_field", "rules": "A response may gain optional fields in any minor version; a client ignores fields it does not know. A required field keeps its name and type until the next major version; a client rejects a body whose required field is missing or mistyped.", "fixtures": [ diff --git a/src/kpubdata_builder/service/app.py b/src/kpubdata_builder/service/app.py index dd19c3ac..d16155de 100644 --- a/src/kpubdata_builder/service/app.py +++ b/src/kpubdata_builder/service/app.py @@ -409,7 +409,10 @@ def _enforce_ownership() -> bool: # declared in the contract; they were answered since 1.19.0/1.20.0 (#994, additive). # 1.81.0 -> 1.82.0: a synchronous POST /build that gets no build slot within the wait # bound answers 429 build_queue_full (#1040, additive). -API_CONTRACT_VERSION = "1.82.0" +# 1.82.0 -> 1.83.0: a run_id is one attempt — the id of a run that ended without a +# manifest is refused to its submitter too: 409 on POST /builds, 400 on POST /build +# (#1042, additive). +API_CONTRACT_VERSION = "1.83.0" #: How long a synchronous ``POST /build`` waits for a build slot before it answers #: ``build_queue_full`` (#1040). Long enough to ride out a short build ahead of it, short diff --git a/src/kpubdata_builder/service/build_runs_api.py b/src/kpubdata_builder/service/build_runs_api.py index f48e066a..96f94273 100644 --- a/src/kpubdata_builder/service/build_runs_api.py +++ b/src/kpubdata_builder/service/build_runs_api.py @@ -547,7 +547,8 @@ def _build_status_from_events(self, run_id: str) -> dict[str, JsonValue] | None: A restart leaves such a run: ``mark_interrupted_runs`` records its failure in the event store only (#683), so neither the registry nor a manifest knows it - and the owner polling it got 404 instead of "submit it again" (#996). The + and the owner polling it got 404 instead of "submit it again" (#996; under a + new run id, #1042). The submission record gives the run's start, the terminal event its end and reason. Only a failed or cancelled ending is reported from here — a run that finished has a manifest, and that path is the authority for it. @@ -667,7 +668,8 @@ def mark_interrupted_runs(self) -> tuple[str, ...]: event="run_failed", status="fail", message=f"{INTERRUPTED_CODE}: the server restarted and the job's " - "provider keys, held only in memory, are gone; submit it again", + "provider keys, held only in memory, are gone; submit it again " + "under a new run_id", ) ) marked.append(run_id) diff --git a/src/kpubdata_builder/service/routes/_guards.py b/src/kpubdata_builder/service/routes/_guards.py index e880dbcb..5d862b7b 100644 --- a/src/kpubdata_builder/service/routes/_guards.py +++ b/src/kpubdata_builder/service/routes/_guards.py @@ -122,7 +122,7 @@ def check_active_run_access( def check_existing_run_access( - service: BuilderService, run_id: str, principal: Principal + service: BuilderService, run_id: str, principal: Principal, *, used_status: int = 409 ) -> ServiceResponse | None: """If the caller-specified run_id **already exists**, verify ownership (#635). @@ -160,12 +160,27 @@ def check_existing_run_access( # id is not free: the event store says who submitted it, and that person can still # read why it ended (#996). Someone else building under the id would read those # events while their job runs, and hand their own failure to the first submitter - # if it is interrupted in turn (#1025). The submitter may use the id again — that - # is what the interrupted run's message asks for. + # if it is interrupted in turn (#1025). + # + # Nor is it free to its own submitter (#1042): a run id is one attempt. A second + # build under it would append its events after the first attempt's ``run_failed``, + # and each attempt's ending would be read as the other's state. A retry takes a new + # id — the same answer a completed run's id gets. submission = service._event_store.submission(run_id) - if submission is not None and not ownership_module.ownership_allows( + if submission is None: + # run_id does not exist yet — new build. + return None + if not ownership_module.ownership_allows( created_by=submission.created_by, owner_id=submission.owner_id, principal=principal ): return ServiceResponse(403, {"error": "forbidden: not run owner"}) - # run_id does not exist yet, or is the caller's own interrupted run — new build. - return None + # ``used_status``: 409 where the route's 409 is an error body (``POST /builds``, like + # a completed run's id). ``POST /build`` declares its 409 as a build response, so it + # asks for 400 — the id cannot be used for this request. + return ServiceResponse( + used_status, + { + "error": "run_id already ended; submit the retry under a new run_id", + "run_id": run_id, + }, + ) diff --git a/src/kpubdata_builder/service/routes/core.py b/src/kpubdata_builder/service/routes/core.py index 0c0033fb..13d2b0bb 100644 --- a/src/kpubdata_builder/service/routes/core.py +++ b/src/kpubdata_builder/service/routes/core.py @@ -91,7 +91,8 @@ def route( # Passing someone else's run_id would overwrite that run's output and return results # in response. Async POST /builds applies the same rule (#991). if run_id is not None: - denied = check_existing_run_access(service, run_id, principal) + # 400 for an id that already ended: this route's 409 is a build response. + denied = check_existing_run_access(service, run_id, principal, used_status=400) if denied is not None: return denied return service.build( diff --git a/tests/unit/test_interrupted_run_status.py b/tests/unit/test_interrupted_run_status.py index dae0fd03..b24d8068 100644 --- a/tests/unit/test_interrupted_run_status.py +++ b/tests/unit/test_interrupted_run_status.py @@ -3,7 +3,7 @@ ``mark_interrupted_runs`` fails such a run in the event store alone. After the restart the job registry is empty and no manifest exists, so the status and events routes answered 404: a client polling its build was told the run did not exist instead of to -submit it again. +submit it again — under a new run id (#1042). """ from __future__ import annotations @@ -207,25 +207,68 @@ def test_another_user_cannot_submit_under_the_interrupted_run_id( assert submission.owner_id == _ALICE.owner_id -def test_the_submitter_can_submit_the_interrupted_run_again( +_USED = { + "error": "run_id already ended; submit the retry under a new run_id", + "run_id": "in-flight", +} + + +def test_the_submitter_cannot_submit_under_the_interrupted_run_id_either( restarted: BuilderService, monkeypatch: pytest.MonkeyPatch ) -> None: - # What the interrupted run's message asks for: send it again. + # A run id is one attempt (#1042, kpubdata#812): a second build under it would + # append its events after the first attempt's ``run_failed``. The retry takes a new + # id — the answer a completed run's id already gets. _as(monkeypatch, _ALICE) + before = _get(restarted, "/builds/in-flight").body response = _submit(restarted, "in-flight") + assert response.status_code == 409 + assert response.body == _USED + assert restarted._async_builds.get("in-flight") is None + # The run still reads as it ended: failed, with the reason it was interrupted. + after = _get(restarted, "/builds/in-flight").body + assert (after["status"], after["code"]) == (before["status"], before["code"]) + assert (after["status"], after["code"]) == ("failed", "credentials_required") + events = cast( + list[dict[str, object]], _get(restarted, "/builds/in-flight/events").body["events"] + ) + assert [event["event"] for event in events].count("run_submitted") == 1 + + +def test_the_synchronous_route_refuses_the_submitter_the_same_way( + restarted: BuilderService, monkeypatch: pytest.MonkeyPatch +) -> None: + _as(monkeypatch, _ALICE) + + response = dispatch(restarted, "POST", "/build", {"spec": _SPEC, "run_id": "in-flight"}) + + assert isinstance(response, ServiceResponse) + # 400, not 409: this route's 409 is a build response with another body (contract). + assert response.status_code == 400 + assert response.body == _USED + + +def test_the_retry_goes_through_under_a_new_run_id( + restarted: BuilderService, monkeypatch: pytest.MonkeyPatch +) -> None: + _as(monkeypatch, _ALICE) + + response = _submit(restarted, "in-flight-retry") + assert response.status_code == 202 - assert response.body["run_id"] == "in-flight" - submission = restarted._event_store.submission("in-flight") - assert submission is not None - assert submission.owner_id == _ALICE.owner_id + assert response.body["run_id"] == "in-flight-retry" -def test_the_submitter_may_use_the_interrupted_run_id_again(restarted: BuilderService) -> None: - from kpubdata_builder.service.routes._guards import check_existing_run_access +def test_the_interrupted_message_asks_for_a_new_run_id( + restarted: BuilderService, monkeypatch: pytest.MonkeyPatch +) -> None: + _as(monkeypatch, _ALICE) - assert check_existing_run_access(restarted, "in-flight", _ALICE) is None + assert str(_get(restarted, "/builds/in-flight").body["error"]).endswith( + "submit it again under a new run_id" + ) def test_an_id_nobody_submitted_is_free_to_anyone(restarted: BuilderService) -> None: