From 6b3f39244b31f55937692f34722f513728ce86c9 Mon Sep 17 00:00:00 2001 From: smaramwbc <145447586+smaramwbc@users.noreply.github.com> Date: Sun, 30 Aug 2026 21:24:07 +0100 Subject: [PATCH] fix(resolutions): refresh the upserted row before serialising it (#367) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /v1/resolutions upserts by (subject_id, session_id, tenant_id), and every write after the first returned 500 while persisting the value. The only upsert-by-logical-key on the consumer surface told callers their write had failed, after committing it. `create_resolution` serialises the row it gets back after committing. `session.commit()` expires every instance in the session, so reading a column off it is lazy IO, which raises MissingGreenlet on the async engine and falls into the catch-all handler as 500/internal_error. It only bit the UPDATE path. `upsert_resolution` returns the instance this request just constructed on INSERT, whose attributes are still populated in Python; on UPDATE it returns the row it loaded from the database, which the commit expired. The fix is the pattern POST /v1/episodes already uses for the same reason: commit, then refresh. resolutions.py was the one router that commits and then reads ORM attributes without refreshing — subjects.py reads only plain ints afterwards, and health.py returns 200 on repeated calls. Not a regression from #295/#356: `from_row` reads the same nine fields the inline mapping read, also after the commit. b99596b~1 behaves identically (200, 500, 500). The defect dates to d4fef67. Not covered by #366 either: that maps OperationalError to 503, and MissingGreenlet is an InvalidRequestError. Three regression tests, all failing on main: repeated writes to one key all return 200; the update response carries the new value rather than a stale first-write body; and the upsert still collapses to one row. --- server/api/resolutions.py | 10 ++- .../test_resolutions_upsert_update.py | 83 +++++++++++++++++++ 2 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 tests/integration/test_resolutions_upsert_update.py diff --git a/server/api/resolutions.py b/server/api/resolutions.py index 7ffca0e..97b59d9 100644 --- a/server/api/resolutions.py +++ b/server/api/resolutions.py @@ -42,6 +42,14 @@ async def create_resolution( result = await repo.upsert_resolution(session, row) await session.commit() + # `commit()` expires every instance in the session, so reading a column off + # `result` afterwards is lazy IO — which raises MissingGreenlet on the async + # engine and surfaces as a 500. It only bites on the UPDATE path: the INSERT + # path returns the instance this request just constructed, whose attributes + # are still populated locally, while the UPDATE path returns the row + # `upsert_resolution` loaded from the database. Same pattern as + # `POST /v1/episodes`, which refreshes for the same reason. + await session.refresh(result) return ResolutionResponse.from_row(result) @@ -63,4 +71,4 @@ async def list_resolutions( rows = await repo.list_resolutions( session, subject_id, tenant_id=tenant_id, status=status, limit=limit, offset=offset ) - return [ResolutionResponse.from_row(r) for r in rows] \ No newline at end of file + return [ResolutionResponse.from_row(r) for r in rows] diff --git a/tests/integration/test_resolutions_upsert_update.py b/tests/integration/test_resolutions_upsert_update.py new file mode 100644 index 0000000..d5706e9 --- /dev/null +++ b/tests/integration/test_resolutions_upsert_update.py @@ -0,0 +1,83 @@ +"""Regression: the UPDATE path of POST /v1/resolutions returned 500. + +`create_resolution` upserts and then serialises the row it gets back. +`session.commit()` expires every instance in the session, so reading a column +off that row afterwards is lazy IO — which raises `MissingGreenlet` on the +async engine and reaches the client as `500 internal_error`. + +It only ever bit the second and later writes to a logical key. The INSERT path +returns the instance the request just constructed, whose attributes are still +populated in Python; the UPDATE path returns the row `upsert_resolution` loaded +from the database, which the commit expired. + +The write itself always landed, so the endpoint persisted the caller's value and +then told them it had failed — the worst of the two possible wrong answers for +the only upsert-by-logical-key on the consumer surface. +""" + +from __future__ import annotations + +import pytest + + +@pytest.mark.anyio +async def test_repeated_writes_to_one_key_all_succeed(client, subject_id): + """Four writes to one (subject_id, session_id) — every one a 200.""" + statuses = ["open", "resolved", "open", "resolved"] + for status in statuses: + response = await client.post( + "/v1/resolutions", + json={ + "subject_id": subject_id, + "session_id": "session-1", + "status": status, + "resolution_summary": f"summary for {status}", + }, + ) + assert response.status_code == 200, f"{status} write returned {response.status_code}" + + +@pytest.mark.anyio +async def test_the_update_response_carries_the_new_value(client, subject_id): + """The response body must describe the write that just happened. + + Asserting only on the status code would pass against a route that returned + a stale first-write body. + """ + await client.post( + "/v1/resolutions", + json={"subject_id": subject_id, "session_id": "s", "status": "open"}, + ) + second = await client.post( + "/v1/resolutions", + json={ + "subject_id": subject_id, + "session_id": "s", + "status": "resolved", + "resolution_summary": "closed it", + }, + ) + + assert second.status_code == 200 + body = second.json() + assert body["status"] == "resolved" + assert body["resolution_summary"] == "closed it" + assert body["resolved_at"] is not None + + +@pytest.mark.anyio +async def test_the_upsert_still_collapses_to_one_row(client, subject_id): + """The point of the endpoint: one logical key, one row, holding the latest.""" + for status in ["open", "resolved", "open"]: + await client.post( + "/v1/resolutions", + json={"subject_id": subject_id, "session_id": "s", "status": status}, + ) + + listed = await client.get("/v1/resolutions", params={"subject_id": subject_id}) + rows = listed.json() + assert len(rows) == 1 + assert rows[0]["status"] == "open" + # An update must not be mistaken for a resolve: `resolved_at` is cleared + # when the status moves back off `resolved`. + assert rows[0]["resolved_at"] is None