From 3a4d3524237e325fc52dd7cebf623d05669b5a40 Mon Sep 17 00:00:00 2001 From: yoyoliuuu Date: Sun, 6 Sep 2026 23:40:30 -0400 Subject: [PATCH] feat: withdraw a waiting submission with POST /submissions/{id}/cancel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first live acceptance test on the P1S left an approved job in the queue with no way to remove it, which is what this closes. Cancel is a queue operation, not an abort. It is legal only from `queued` and `approved` (`CANCELLABLE_STATES`), reaches no printer, and is refused for anything past the queue: stopping a running print is a control-plane action that needs a claim, and this surface has none. A test pins that `cancelled` is unreachable from `dispatching` / `running` / `finished` / `failed` / `rejected`. Cancelling deletes the stored artifact -- a withdrawn job has no further use for it, and it is the submitter's data -- and marks `artifact_removed`. The job record stays, with the actor and any reason in its history, so the withdrawal remains auditable. An approved job that is cancelled has `dispatch_ready` retracted, or the record would still read as cleared to run. There is no undo; a withdrawn job is resubmitted, not revived. Note this goes beyond the design's §5 state machine, which declares no `cancelled` state. Recorded in docs/TODO.md as an addition to fold back into SUBMISSION_PIPELINE_DESIGN.md when that doc is next revised. `uv run ruff check .` and `uv run pytest -q` (126 tests) pass. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UQvsfEeDitEyNzbCwrcEdD --- README.md | 16 +++++++ docs/TODO.md | 10 +++-- src/bambu_server/main.py | 42 ++++++++++++++++++ src/bambu_server/submissions.py | 66 +++++++++++++++++++++++++++- tests/test_submission_api.py | 72 ++++++++++++++++++++++++++++++ tests/test_submissions.py | 77 +++++++++++++++++++++++++++++++++ 6 files changed, 278 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index bcdfa26..4c16494 100644 --- a/README.md +++ b/README.md @@ -102,6 +102,7 @@ Submission pipeline routes: | GET | `/submissions` | List jobs (`machine`, `state`, `limit` filters) | | GET | `/submissions/{submission_id}` | One job with its verdict and history | | POST | `/submissions/{submission_id}/approve` | Record sign-off on a queued job | +| POST | `/submissions/{submission_id}/cancel` | Withdraw a waiting job from the queue | No `/control/*` routes exist, and no route dispatches a print. @@ -174,6 +175,7 @@ wait in a per-machine queue with expected finish times. The design contract is ```text submitted -> validating -> validated -> queued -> approved -> | dispatch \-> rejected (terminal) | not implemented + \--------\-> cancelled (terminal) ``` Everything up to and including approval is analysis and bookkeeping. Approval is @@ -274,6 +276,20 @@ history so decisions become attributable the moment a real identity provider Approval is human-in-the-loop by design: nothing auto-approves, and a submission that did not pass validation can never be approved. +### Cancelling + +`POST /submissions/{id}/cancel` withdraws a waiting job. It is a **queue +operation, not an abort**: it is legal only from `queued` and `approved`, it +reaches no printer, and it is deliberately refused for anything past the queue — +stopping a running print is a control-plane action that needs a claim, and this +surface has none. + +Cancelling deletes the stored artifact (a withdrawn job has no further use for +it, and it is the submitter's data) and marks `artifact_removed`. The job record +stays, with the actor and reason in its history, so the withdrawal remains +auditable. There is no undo — a withdrawn job is resubmitted, not revived. An +approved job that is cancelled has its `dispatch_ready` retracted. + ## Dashboard registration Add one entry per printer to `ac-organic-lab/equipment.yaml` after deploying the diff --git a/docs/TODO.md b/docs/TODO.md index 6952e3d..6c23061 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -76,8 +76,12 @@ Open, from the design's §10 data gaps and what the build surfaced: slow (the read is on a worker thread, so it does not stall status polls). If that becomes a problem, move it to an async worker — the state machine already has the `validating` state for it. -- No cancel/withdraw path exists for a queued job; the contract's state machine - declares none. +- **Beyond the design's state machine:** a `cancelled` terminal state and + `POST /submissions/{id}/cancel` were added after the first live test left an + unremovable job in the P1S queue. The contract's §5 declares no such state. + It is legal only from `queued` / `approved`, never for a dispatched job — + aborting a print stays a control-plane action. Worth folding back into + `SUBMISSION_PIPELINE_DESIGN.md` §5 when that doc is next revised. - No retention policy: rejected and finished jobs, and their uploaded artifacts, stay on disk and in `GET /submissions` indefinitely. Fine at current volume, but it needs a sweep before this runs unattended for long. @@ -85,7 +89,7 @@ Open, from the design's §10 data gaps and what the build surfaced: ## Test suite - `uv run ruff check .` passes. -- `uv run pytest -q` passes all 114 tests, including the FastAPI API tests and +- `uv run pytest -q` passes all 126 tests, including the FastAPI API tests and the submission pipeline (artifact inspection, validation, store/state machine, queue ETA, HTTP surface). Tests build their own `.3mf` and `.gcode` fixtures and use fake backends; nothing touches hardware. diff --git a/src/bambu_server/main.py b/src/bambu_server/main.py index 1ff8a0d..5374f8a 100644 --- a/src/bambu_server/main.py +++ b/src/bambu_server/main.py @@ -80,6 +80,17 @@ class ApprovalRequest(BaseModel): approved_by: str = Field(min_length=1, max_length=120) +class CancellationRequest(BaseModel): + """Withdrawal of a waiting submission. + + ``cancelled_by`` is opaque, on the same terms as ``approved_by``. ``reason`` + is free text kept in the job's history so a withdrawal is explicable later. + """ + + cancelled_by: str = Field(min_length=1, max_length=120) + reason: str | None = Field(default=None, max_length=500) + + def create_app( *, settings: Settings | None = None, @@ -391,6 +402,37 @@ async def approve_submission( except InvalidTransition as exc: raise HTTPException(status_code=409, detail=str(exc)) from exc + @app.post( + "/submissions/{submission_id}/cancel", + response_model=SubmissionJob, + tags=["submissions"], + ) + async def cancel_submission( + store: Annotated[SubmissionStore, Depends(get_store)], + submission_id: Annotated[str, Path(pattern=r"^[0-9a-f]{32}$")], + cancellation: CancellationRequest, + ) -> SubmissionJob: + """Withdraw a waiting submission from its machine's queue. + + A queue operation, not an abort: it reaches no printer and is refused + for anything past the queue. Deletes the stored artifact and keeps the + job record, so the withdrawal stays auditable. + """ + + job = store.get(submission_id) + if job is None: + raise HTTPException(status_code=404, detail="unknown submission") + try: + return await store.cancel( + job, + cancelled_by=cancellation.cancelled_by, + reason=cancellation.reason, + ) + except InvalidTransition as exc: + raise HTTPException(status_code=409, detail=str(exc)) from exc + except SubmissionError as exc: + raise HTTPException(status_code=400, detail=str(exc)) from exc + return app diff --git a/src/bambu_server/submissions.py b/src/bambu_server/submissions.py index 652fe88..ccf5a69 100644 --- a/src/bambu_server/submissions.py +++ b/src/bambu_server/submissions.py @@ -57,18 +57,25 @@ "finished", "failed", "rejected", + "cancelled", ] #: Which states are waiting in a machine's queue. ``approved`` stays queued #: because approval alone moves nothing -- only dispatch does. QUEUED_STATES: frozenset[str] = frozenset({"queued", "approved"}) +#: States a submitter or operator may withdraw a job from. Deliberately only +#: the waiting ones: cancelling means "take this out of the queue", never "stop +#: a print". A job that has reached the printer is the control plane's problem, +#: and abort belongs there under a claim -- not on this surface. +CANCELLABLE_STATES: frozenset[str] = frozenset({"queued", "approved"}) + ALLOWED_TRANSITIONS: dict[str, frozenset[str]] = { "submitted": frozenset({"validating", "failed"}), "validating": frozenset({"validated", "rejected", "failed"}), "validated": frozenset({"queued", "failed"}), - "queued": frozenset({"approved", "failed"}), - "approved": frozenset({"dispatching", "failed"}), + "queued": frozenset({"approved", "cancelled", "failed"}), + "approved": frozenset({"dispatching", "cancelled", "failed"}), "dispatching": frozenset({"running", "failed"}), "running": frozenset({"finished", "failed"}), # Terminal. `failed` is deliberately terminal too: the contract sends it to @@ -77,6 +84,7 @@ "finished": frozenset(), "failed": frozenset(), "rejected": frozenset(), + "cancelled": frozenset(), } _CONTROL_CHARS = re.compile(r"[\x00-\x1f\x7f]") @@ -154,6 +162,10 @@ class SubmissionJob(BaseModel): updated_at: datetime approved_by: str | None = None approved_at: datetime | None = None + #: True once the stored artifact has been deleted (cancellation). The job + #: record outlives its file so the audit trail survives, but nothing can be + #: run from it afterwards. + artifact_removed: bool = False estimated_duration_minutes: float | None = None facts: ArtifactFacts | None = None verdict: ValidationVerdict | None = None @@ -368,6 +380,56 @@ async def approve(self, job: SubmissionJob, *, approved_by: str) -> SubmissionJo updated.submission_id, "approved", f"approved by {updated.approved_by}" ) + async def cancel( + self, + job: SubmissionJob, + *, + cancelled_by: str, + reason: str | None = None, + ) -> SubmissionJob: + """Withdraw a waiting job from its machine's queue. + + Legal only from the states in :data:`CANCELLABLE_STATES`. This is a + queue operation, **not** an abort: it can never reach a printer, and it + is deliberately not offered for a job that has been dispatched -- + stopping a running print is a control-plane action that needs a claim. + + The stored artifact is deleted, because a withdrawn job has no further + use for it and it is the submitter's data. The job record stays, so the + decision and its reason remain auditable; ``artifact_removed`` marks + that the file is gone. There is no undo -- a withdrawn job is + resubmitted, not revived. + """ + + async with self._lock: + current = self._require(job.submission_id) + if current.state not in CANCELLABLE_STATES: + raise InvalidTransition( + f"only a waiting submission can be cancelled; " + f"{current.submission_id} is {current.state}" + ) + + path = self.artifact_path(current) + await asyncio.to_thread(path.unlink, True) + + who = clean_text(cancelled_by, field="cancelled_by", required=True) + why = clean_text(reason, field="reason", required=False) + update: dict[str, object] = {"artifact_removed": True} + if current.verdict is not None: + # An approved job carries dispatch_ready; withdrawing it must + # retract that, or the record would still read as cleared to run. + update["verdict"] = current.verdict.model_copy( + update={"dispatch_ready": False} + ) + self._jobs[current.submission_id] = current.model_copy(update=update) + + note = f"cancelled by {who}" + if why: + note = f"{note}: {why}" + return await self._transition_locked( + current.submission_id, "cancelled", note + ) + # -- internals --------------------------------------------------------- def _require(self, submission_id: str) -> SubmissionJob: diff --git a/tests/test_submission_api.py b/tests/test_submission_api.py index 6d07f4c..e505205 100644 --- a/tests/test_submission_api.py +++ b/tests/test_submission_api.py @@ -294,3 +294,75 @@ def test_reads_never_cause_printer_io(client: TestClient, backend: FakeBackend) client.get("/submissions") assert backend.read_count == before + + +def test_cancelling_removes_a_job_from_the_queue(client: TestClient) -> None: + created = _upload(client).json() + assert len(client.get("/printers/bambu_test_01/queue").json()["queued"]) == 1 + + response = client.post( + f"/submissions/{created['submission_id']}/cancel", + json={"cancelled_by": "lab-operator", "reason": "superseded"}, + ) + + assert response.status_code == 200 + job = response.json() + assert job["state"] == "cancelled" + assert job["artifact_removed"] is True + assert job["history"][-1]["note"] == "cancelled by lab-operator: superseded" + assert client.get("/printers/bambu_test_01/queue").json()["queued"] == [] + # The record survives so the withdrawal stays auditable. + assert client.get(f"/submissions/{created['submission_id']}").status_code == 200 + + +def test_cancelling_an_approved_job_is_allowed(client: TestClient) -> None: + created = _upload(client).json() + client.post( + f"/submissions/{created['submission_id']}/approve", + json={"approved_by": "lab-operator"}, + ) + + response = client.post( + f"/submissions/{created['submission_id']}/cancel", + json={"cancelled_by": "lab-operator"}, + ) + + assert response.status_code == 200 + assert response.json()["verdict"]["dispatch_ready"] is False + + +def test_cancelling_twice_is_a_conflict(client: TestClient) -> None: + created = _upload(client).json() + path = f"/submissions/{created['submission_id']}/cancel" + client.post(path, json={"cancelled_by": "lab-operator"}) + + assert client.post(path, json={"cancelled_by": "lab-operator"}).status_code == 409 + + +def test_cancelling_a_rejected_submission_is_a_conflict(client: TestClient) -> None: + body = SAMPLE_GCODE.replace("; nozzle_diameter = 0.4", "; nozzle_diameter = 0.6") + created = _upload(client, body=body.encode()).json() + + response = client.post( + f"/submissions/{created['submission_id']}/cancel", + json={"cancelled_by": "lab-operator"}, + ) + assert response.status_code == 409 + + +def test_cancelling_an_unknown_submission_is_404(client: TestClient) -> None: + response = client.post( + "/submissions/" + "0" * 32 + "/cancel", json={"cancelled_by": "lab-operator"} + ) + assert response.status_code == 404 + + +def test_a_cancelled_job_can_be_filtered_for(client: TestClient) -> None: + created = _upload(client).json() + client.post( + f"/submissions/{created['submission_id']}/cancel", + json={"cancelled_by": "lab-operator"}, + ) + + assert len(client.get("/submissions", params={"state": "cancelled"}).json()) == 1 + assert client.get("/submissions", params={"state": "queued"}).json() == [] diff --git a/tests/test_submissions.py b/tests/test_submissions.py index 16301cb..39e62a3 100644 --- a/tests/test_submissions.py +++ b/tests/test_submissions.py @@ -300,3 +300,80 @@ async def test_a_blank_material_is_treated_as_absent(store: SubmissionStore) -> original_filename="part.gcode", ) assert job.material is None + + +async def test_cancelling_a_queued_job_removes_it_and_its_artifact( + store: SubmissionStore, profile: MachineProfile +) -> None: + job = await run_validation(store, await _accept(store), profile) + path = store.artifact_path(job) + assert path.exists() + + cancelled = await store.cancel(job, cancelled_by="operator", reason="wrong plate") + + assert cancelled.state == "cancelled" + assert cancelled.artifact_removed is True + assert not path.exists() + assert store.queue_for("bambu_test_01") == [] + # The record outlives the file, so the withdrawal stays explicable. + assert cancelled.history[-1].note == "cancelled by operator: wrong plate" + assert store.get(job.submission_id) is not None + + +async def test_cancelling_an_approved_job_retracts_dispatch_ready( + store: SubmissionStore, profile: MachineProfile +) -> None: + job = await run_validation(store, await _accept(store), profile) + approved = await store.approve(job, approved_by="operator") + assert approved.verdict.dispatch_ready is True + + cancelled = await store.cancel(approved, cancelled_by="operator") + + assert cancelled.state == "cancelled" + # A withdrawn job must not still read as cleared to run. + assert cancelled.verdict.dispatch_ready is False + + +async def test_cancellation_is_terminal_and_not_repeatable( + store: SubmissionStore, profile: MachineProfile +) -> None: + job = await run_validation(store, await _accept(store), profile) + cancelled = await store.cancel(job, cancelled_by="operator") + + for attempt in ( + store.cancel(cancelled, cancelled_by="operator"), + store.approve(cancelled, approved_by="operator"), + ): + with pytest.raises(InvalidTransition): + await attempt + + +async def test_a_rejected_job_cannot_be_cancelled( + store: SubmissionStore, profile: MachineProfile +) -> None: + """Rejection is already terminal; cancelling it would muddy the record.""" + body = SAMPLE_GCODE.replace("; nozzle_diameter = 0.4", "; nozzle_diameter = 0.6") + job = await run_validation(store, await _accept(store, body.encode()), profile) + + with pytest.raises(InvalidTransition, match="only a waiting submission"): + await store.cancel(job, cancelled_by="operator") + + +async def test_cancel_refuses_a_blank_actor( + store: SubmissionStore, profile: MachineProfile +) -> None: + job = await run_validation(store, await _accept(store), profile) + + with pytest.raises(SubmissionError, match="cancelled_by"): + await store.cancel(job, cancelled_by=" ") + + assert store.get(job.submission_id).state == "queued" + + +def test_cancellation_can_never_reach_a_dispatched_job() -> None: + """Cancel is a queue operation; stopping a print is the control plane's job.""" + from bambu_server.submissions import ALLOWED_TRANSITIONS, CANCELLABLE_STATES + + assert CANCELLABLE_STATES == {"queued", "approved"} + for state in ("dispatching", "running", "finished", "failed", "rejected"): + assert "cancelled" not in ALLOWED_TRANSITIONS[state]