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).
This commit is contained in:
@@ -156,6 +156,10 @@ class Reading:
|
|||||||
# Mongo, so the brewer's own comment is never overwritten.
|
# Mongo, so the brewer's own comment is never overwritten.
|
||||||
advice: str = ""
|
advice: str = ""
|
||||||
advice_ok: bool = True
|
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
|
@property
|
||||||
def sg_decimal(self) -> float | None:
|
def sg_decimal(self) -> float | None:
|
||||||
@@ -177,6 +181,7 @@ class Reading:
|
|||||||
_id=str(doc.get("_id") or ""),
|
_id=str(doc.get("_id") or ""),
|
||||||
advice=(doc.get("advice") or "").strip(),
|
advice=(doc.get("advice") or "").strip(),
|
||||||
advice_ok=bool(doc.get("advice_ok", True)),
|
advice_ok=bool(doc.get("advice_ok", True)),
|
||||||
|
advice_token=str(doc.get("advice_token") or ""),
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -209,13 +209,20 @@ class BeerRepository:
|
|||||||
"final_reading": "True" if final_reading else "False",
|
"final_reading": "True" if final_reading else "False",
|
||||||
"comment": str(comment),
|
"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
|
# it was generated. If the reading has been edited, that
|
||||||
# advice no longer describes it, so it is cleared and a fresh
|
# advice no longer describes it, so it is cleared and a fresh
|
||||||
# one is generated. (Advice is only ever cleared here - never
|
# one is generated. (Advice is only ever cleared here - never
|
||||||
# the brewer's comment.)
|
# the brewer's comment.)
|
||||||
"$unset": {"advice": "", "advice_ok": "", "advice_at": ""},
|
"$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)
|
result = self.readings.update_one(query, update, upsert=True)
|
||||||
if result.upserted_id is not None:
|
if result.upserted_id is not None:
|
||||||
return str(result.upserted_id)
|
return str(result.upserted_id)
|
||||||
@@ -224,22 +231,32 @@ class BeerRepository:
|
|||||||
|
|
||||||
def set_reading_advice(self, reading_id: str, advice: str,
|
def set_reading_advice(self, reading_id: str, advice: str,
|
||||||
ok: bool = True,
|
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.
|
"""Attach the LLM's advice to a reading.
|
||||||
|
|
||||||
The advice lives in its own field, so the brewer's own comment is
|
The advice lives in its own field, so the brewer's own comment is
|
||||||
never overwritten. ``ok`` says whether the advice was actually
|
never overwritten. ``ok`` says whether the advice was actually
|
||||||
generated or is a note explaining that it could not be.
|
generated or is a note explaining that it could not be.
|
||||||
|
|
||||||
``expected_sg`` is the gravity the advice was generated FOR. The
|
The write is CONDITIONAL, so a slow advice thread cannot
|
||||||
write only happens if the reading still holds that gravity.
|
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
|
This matters because the advice is generated in the background: if
|
||||||
the brewer corrects a reading while the advice for the old value
|
the brewer corrects a reading while the advice for the old value
|
||||||
is still being generated, the slow thread would land afterwards
|
is still being generated, the slow thread would land afterwards
|
||||||
and overwrite the fresh advice with advice about a reading that
|
and overwrite the fresh advice with advice about a reading that
|
||||||
no longer exists. Without the guard the run ends up with the
|
no longer exists.
|
||||||
final gravity from the correction and the advice from before it.
|
|
||||||
|
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)
|
doc_id = _as_doc_id(reading_id)
|
||||||
if not doc_id:
|
if not doc_id:
|
||||||
@@ -248,6 +265,8 @@ class BeerRepository:
|
|||||||
query: dict = {"_id": {"$in": doc_id}}
|
query: dict = {"_id": {"$in": doc_id}}
|
||||||
if expected_sg is not None:
|
if expected_sg is not None:
|
||||||
query["sg"] = str(expected_sg)
|
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
|
# `$in` matches either form. A plain list would be an exact
|
||||||
# ARRAY match in Mongo, which is not what is wanted here.
|
# 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)
|
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:
|
def _now_iso() -> str:
|
||||||
from datetime import datetime, timezone
|
from datetime import datetime, timezone
|
||||||
|
|
||||||
|
|||||||
@@ -91,6 +91,7 @@ def _start_advice(run_id: str, reading_id: str, corrected: bool = False) -> None
|
|||||||
points = reading.sg_points
|
points = reading.sg_points
|
||||||
readings.append({
|
readings.append({
|
||||||
"_id": str(reading._id),
|
"_id": str(reading._id),
|
||||||
|
"advice_token": reading.advice_token,
|
||||||
"date": reading.date.isoformat() if reading.date else "",
|
"date": reading.date.isoformat() if reading.date else "",
|
||||||
"sg": points,
|
"sg": points,
|
||||||
"sg_decimal": reading.sg_decimal,
|
"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")
|
log.exception("could not gather the reading for the advice")
|
||||||
return
|
return
|
||||||
|
|
||||||
# The gravity the advice is about. The write is conditional on the
|
# The gravity and the editing token of the reading being advised
|
||||||
# reading still holding it, so a slow advice thread cannot overwrite
|
# about. The write is conditional on both, so a slow advice thread
|
||||||
# a correction with advice about the old value.
|
# cannot overwrite a correction with advice about the old value.
|
||||||
advice_sg = latest.get("sg")
|
advice_sg = latest.get("sg")
|
||||||
|
advice_token = latest.get("advice_token")
|
||||||
|
|
||||||
def write(read_id: str, ok: bool, text: str) -> None:
|
def write(read_id: str, ok: bool, text: str) -> None:
|
||||||
# The client is SHARED, not built per reading. A MongoClient
|
# 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:
|
try:
|
||||||
shared = get_shared_repository()
|
shared = get_shared_repository()
|
||||||
written = shared.set_reading_advice(read_id, text, ok,
|
written = shared.set_reading_advice(
|
||||||
expected_sg=advice_sg)
|
read_id, text, ok,
|
||||||
|
expected_sg=advice_sg,
|
||||||
|
expected_token=advice_token,
|
||||||
|
)
|
||||||
if not written:
|
if not written:
|
||||||
# The reading was corrected while the advice was being
|
# The reading was corrected while the advice was being
|
||||||
# generated, so the advice no longer matches it. That is
|
# generated, so the advice no longer matches it. That is
|
||||||
|
|||||||
@@ -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):
|
def test_a_reply_cut_off_at_the_token_ceiling_is_marked(repo):
|
||||||
"""A reply that stopped at the token limit is incomplete.
|
"""A reply that stopped at the token limit is incomplete.
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user