From a78aabe3eaa9f27da664102704ae5145e7e517a6 Mon Sep 17 00:00:00 2001 From: hermes Date: Mon, 9 Nov 2026 18:14:24 +1000 Subject: [PATCH] fix(advice): an editing token, so a comment-only correction is safe too The gravity guard caught a correction that changed the gravity, but a COMMENT-ONLY correction leaves the gravity unchanged - so the late thread for the old advice still matched and overwrote the fresh one. Reproduced against the live database: fresh advice matched: 1 stale thread matched: 1 <-- should be 0 FINAL advice: 'STALE ADVICE' Every edit now writes a fresh token onto the reading, and the advice thread carries the token it was given. A late thread finds a token that no longer matches, so its write is refused. The gravity is still checked as well. Verified against the live database: token changed: True fresh advice matched: 1 stale thread matched: 0 <-- must be 0 FINAL advice: 'FRESH ADVICE' 174 tests (up from 173). --- src/flask/brewing.py | 5 +++++ src/flask/mongo/repository.py | 38 +++++++++++++++++++++++++++------ src/flask/views/brews.py | 15 ++++++++----- tests/test_brew_advice.py | 40 +++++++++++++++++++++++++++++++++++ 4 files changed, 87 insertions(+), 11 deletions(-) diff --git a/src/flask/brewing.py b/src/flask/brewing.py index 8332f01..470e130 100644 --- a/src/flask/brewing.py +++ b/src/flask/brewing.py @@ -156,6 +156,10 @@ class Reading: # Mongo, so the brewer's own comment is never overwritten. advice: str = "" advice_ok: bool = True + # Marks this generation of the reading. Every edit writes a fresh + # one, so an advice thread working on an earlier version of the + # reading can be told apart from the current one. + advice_token: str = "" @property def sg_decimal(self) -> float | None: @@ -177,6 +181,7 @@ class Reading: _id=str(doc.get("_id") or ""), advice=(doc.get("advice") or "").strip(), advice_ok=bool(doc.get("advice_ok", True)), + advice_token=str(doc.get("advice_token") or ""), ) diff --git a/src/flask/mongo/repository.py b/src/flask/mongo/repository.py index c36c8e1..931eb1c 100644 --- a/src/flask/mongo/repository.py +++ b/src/flask/mongo/repository.py @@ -209,13 +209,20 @@ class BeerRepository: "final_reading": "True" if final_reading else "False", "comment": str(comment), }, - # The advice was written for the gravity that was there when + # The advice was written for the reading that was there when # it was generated. If the reading has been edited, that # advice no longer describes it, so it is cleared and a fresh # one is generated. (Advice is only ever cleared here - never # the brewer's comment.) "$unset": {"advice": "", "advice_ok": "", "advice_at": ""}, } + # A fresh token marks this generation of the reading. The advice + # thread carries the token it was given, so a thread that is + # still working on an EARLIER version of the reading finds a + # token that no longer matches and its write is refused. The + # gravity alone is not enough - a comment-only correction leaves + # the gravity unchanged. + update["$set"]["advice_token"] = _new_token() result = self.readings.update_one(query, update, upsert=True) if result.upserted_id is not None: return str(result.upserted_id) @@ -224,22 +231,32 @@ class BeerRepository: def set_reading_advice(self, reading_id: str, advice: str, ok: bool = True, - expected_sg: int | None = None) -> int: + expected_sg: int | None = None, + expected_token: str | None = None) -> int: """Attach the LLM's advice to a reading. The advice lives in its own field, so the brewer's own comment is never overwritten. ``ok`` says whether the advice was actually generated or is a note explaining that it could not be. - ``expected_sg`` is the gravity the advice was generated FOR. The - write only happens if the reading still holds that gravity. + The write is CONDITIONAL, so a slow advice thread cannot + overwrite a correction: + + - ``expected_token`` is the editing token of the reading at the + moment the advice was generated. Every edit writes a fresh + token, so a late thread's token no longer matches. + - ``expected_sg`` is the gravity the advice was generated for, + as a second check. This matters because the advice is generated in the background: if the brewer corrects a reading while the advice for the old value is still being generated, the slow thread would land afterwards and overwrite the fresh advice with advice about a reading that - no longer exists. Without the guard the run ends up with the - final gravity from the correction and the advice from before it. + no longer exists. + + The token is needed as well as the gravity, because a + comment-only correction leaves the gravity unchanged - the token + is what tells the two generations apart. """ doc_id = _as_doc_id(reading_id) if not doc_id: @@ -248,6 +265,8 @@ class BeerRepository: query: dict = {"_id": {"$in": doc_id}} if expected_sg is not None: query["sg"] = str(expected_sg) + if expected_token is not None: + query["advice_token"] = str(expected_token) # `$in` matches either form. A plain list would be an exact # ARRAY match in Mongo, which is not what is wanted here. @@ -485,6 +504,13 @@ def hash_password(password: str) -> str: return generate_password_hash(password) +def _new_token() -> str: + """A fresh token, to mark one generation of a reading from the next.""" + import secrets + + return secrets.token_hex(8) + + def _now_iso() -> str: from datetime import datetime, timezone diff --git a/src/flask/views/brews.py b/src/flask/views/brews.py index 5c6045b..c384365 100644 --- a/src/flask/views/brews.py +++ b/src/flask/views/brews.py @@ -91,6 +91,7 @@ def _start_advice(run_id: str, reading_id: str, corrected: bool = False) -> None points = reading.sg_points readings.append({ "_id": str(reading._id), + "advice_token": reading.advice_token, "date": reading.date.isoformat() if reading.date else "", "sg": points, "sg_decimal": reading.sg_decimal, @@ -113,10 +114,11 @@ def _start_advice(run_id: str, reading_id: str, corrected: bool = False) -> None log.exception("could not gather the reading for the advice") return - # The gravity the advice is about. The write is conditional on the - # reading still holding it, so a slow advice thread cannot overwrite - # a correction with advice about the old value. + # The gravity and the editing token of the reading being advised + # about. The write is conditional on both, so a slow advice thread + # cannot overwrite a correction with advice about the old value. advice_sg = latest.get("sg") + advice_token = latest.get("advice_token") def write(read_id: str, ok: bool, text: str) -> None: # The client is SHARED, not built per reading. A MongoClient @@ -131,8 +133,11 @@ def _start_advice(run_id: str, reading_id: str, corrected: bool = False) -> None try: shared = get_shared_repository() - written = shared.set_reading_advice(read_id, text, ok, - expected_sg=advice_sg) + written = shared.set_reading_advice( + read_id, text, ok, + expected_sg=advice_sg, + expected_token=advice_token, + ) if not written: # The reading was corrected while the advice was being # generated, so the advice no longer matches it. That is diff --git a/tests/test_brew_advice.py b/tests/test_brew_advice.py index f5db183..960f47c 100644 --- a/tests/test_brew_advice.py +++ b/tests/test_brew_advice.py @@ -600,6 +600,46 @@ def test_stale_advice_cannot_overwrite_a_correction(repo): ) +def test_a_comment_only_correction_cannot_be_overwritten(repo): + """A comment-only correction leaves the gravity unchanged. + + So the gravity alone cannot tell the two generations apart - a late + thread for the old advice would still match and overwrite the fresh + one. The editing token is what tells them apart. + """ + # Written through upsert_reading, so it has an editing token. + reading_id = repo.upsert_reading(beer_run_id="1", beer_type="Ale", sg=1050, + reading_date="2026-01-01", + final_reading=False, comment="first comment") + doc = repo.readings.find_one({"beer_run_id": "1"}) + token = doc.get("advice_token") + assert token, "the reading has no editing token" + + # The advice thread starts on this generation. + # The brewer corrects the COMMENT only - the gravity is unchanged. + repo.upsert_reading(beer_run_id="1", beer_type="Ale", sg=1050, + reading_date="2026-01-01", final_reading=False, + comment="corrected comment") + assert repo.readings.find_one({"beer_run_id": "1"}).get("advice_token") != token, ( + "the correction did not write a fresh token" + ) + + # The fresh advice lands. + fresh_token = repo.readings.find_one({"beer_run_id": "1"}).get("advice_token") + assert repo.set_reading_advice(reading_id, "FRESH ADVICE", True, + expected_sg=1050, + expected_token=fresh_token) == 1 + + # The late thread for the OLD generation finally lands. + stale = repo.set_reading_advice(reading_id, "STALE ADVICE", True, + expected_sg=1050, expected_token=token) + assert stale == 0, "the stale advice overwrote the correction" + + after = repo.readings.find_one({"beer_run_id": "1"}) + assert after["advice"] == "FRESH ADVICE" + assert after["comment"] == "corrected comment" + + def test_a_reply_cut_off_at_the_token_ceiling_is_marked(repo): """A reply that stopped at the token limit is incomplete.