fix(advice): a race in the shared repository, and the prompt naming the wrong reading
Two findings from the re-review of PR #14. A race that recreated the leak ------------------------------- `get_shared_repository()` did an unsynchronised check-then-set on a module global. Eight concurrent advice threads on a cold process each saw None and each built their own repository - so eight MongoClients and twenty-five threads. The fix that was meant to remove the leak reintroduced it, just bounded by a race instead of by the number of readings. Measured before: 8 repositories built, 25 threads. Measured after: 1 repository built, 4 threads. There is a lock, and the check is repeated inside it. The prompt was about the wrong reading -------------------------------------- The advice targeted the right reading, but the PROMPT said "give advice for the most recent reading" - and when the brewer corrects an OLDER reading, the most recent one in the history is a different reading. So the model was asked about the wrong number while the advice was filed against the right one. The reading is now named in the instruction (with its date and gravity), marked in the reading list as the one to advise on, and the system prompt says to advise about the marked reading because it may not be the most recent one. A corrected reading is also flagged as such, so the model knows it may differ from what it said before. 167 tests (up from 164).
This commit is contained in:
@@ -56,6 +56,46 @@ def test_the_prompt_tells_the_model_not_to_call_a_reading_impossible():
|
||||
assert "still be real" in brew_advice.SYSTEM_PROMPT
|
||||
|
||||
|
||||
def test_the_prompt_names_the_reading_to_advise_about():
|
||||
"""The reading being advised about must be NAMED.
|
||||
|
||||
The brewer may be correcting an OLDER reading, so "the most recent
|
||||
reading" is the wrong one - and the advice would be about the wrong
|
||||
number. The reading is marked in the list and named in the
|
||||
instruction.
|
||||
"""
|
||||
run = {"canonical_name": "Ale"}
|
||||
older = {"_id": "old", "date": "2026-01-01", "sg_decimal": 1.042,
|
||||
"point_scale": "1042 points", "comment": "", "final": False}
|
||||
newer = {"_id": "new", "date": "2026-01-10", "sg_decimal": 1.030,
|
||||
"point_scale": "1030 points", "comment": "", "final": False}
|
||||
|
||||
prompt = brew_advice.build_prompt(run, older, [older, newer])
|
||||
|
||||
# The older one is the subject.
|
||||
assert "THIS IS THE READING TO ADVISE ON" in prompt
|
||||
# And it is named, with its date and gravity.
|
||||
assert "2026-01-01" in prompt
|
||||
assert "1.042" in prompt
|
||||
# And NOT the newer one.
|
||||
assert "1.03" in prompt # it is in the history, which is right
|
||||
assert prompt.count("THIS IS THE READING TO ADVISE ON") == 1
|
||||
|
||||
|
||||
def test_the_prompt_says_when_a_reading_has_been_corrected():
|
||||
"""A corrected reading may differ from what it said before."""
|
||||
run = {"canonical_name": "Ale"}
|
||||
reading = {"_id": "r1", "date": "2026-01-01", "sg_decimal": 1.012,
|
||||
"point_scale": "1012 points", "comment": "", "final": False}
|
||||
|
||||
plain = brew_advice.build_prompt(run, reading, [reading])
|
||||
assert "just corrected" not in plain
|
||||
|
||||
corrected = brew_advice.build_prompt(run, reading, [reading], corrected=True)
|
||||
assert "just corrected" in corrected
|
||||
assert "may differ" in corrected
|
||||
|
||||
|
||||
def test_the_prompt_marks_the_final_reading():
|
||||
run = {"canonical_name": "Ale"}
|
||||
reading = {"date": "2026-01-20", "sg_decimal": 1.010, "point_scale": "1010 points",
|
||||
@@ -361,9 +401,10 @@ def test_the_advice_is_about_the_reading_that_was_saved():
|
||||
|
||||
captured = {}
|
||||
|
||||
def fake_generate(run, reading, history, writer=None):
|
||||
def fake_generate(run, reading, history, writer=None, corrected=False):
|
||||
captured["reading"] = reading
|
||||
captured["history"] = history
|
||||
captured["corrected"] = corrected
|
||||
return True, "THE ADVICE"
|
||||
|
||||
original = brews_module.brew_advice.generate_advice_in_background
|
||||
@@ -396,6 +437,53 @@ def test_the_advice_is_about_the_reading_that_was_saved():
|
||||
)
|
||||
|
||||
|
||||
def test_the_shared_repository_is_built_only_once():
|
||||
"""Concurrent advice threads must not each build their own client.
|
||||
|
||||
An unsynchronised check-then-set on a module global let eight threads
|
||||
on a cold process each build their own repository - so the fix
|
||||
reintroduced the very leak it was meant to remove, just bounded by a
|
||||
race instead of by the number of readings.
|
||||
"""
|
||||
import threading
|
||||
|
||||
import mongomock
|
||||
import mongo.repository as R
|
||||
|
||||
original_shared = R._shared_repository
|
||||
built = []
|
||||
original_init = R.BeerRepository.__init__
|
||||
|
||||
def counting_init(self, *args, **kwargs):
|
||||
built.append(1)
|
||||
original_init(self, *args, **kwargs)
|
||||
|
||||
R._shared_repository = None
|
||||
R.BeerRepository.__init__ = counting_init
|
||||
original_build = R._build_client
|
||||
R._build_client = lambda: mongomock.MongoClient()
|
||||
try:
|
||||
instances = []
|
||||
barrier = threading.Barrier(8)
|
||||
|
||||
def go():
|
||||
barrier.wait() # release all eight at once
|
||||
instances.append(id(R.get_shared_repository()))
|
||||
|
||||
threads = [threading.Thread(target=go) for _ in range(8)]
|
||||
for t in threads:
|
||||
t.start()
|
||||
for t in threads:
|
||||
t.join()
|
||||
finally:
|
||||
R._shared_repository = original_shared
|
||||
R.BeerRepository.__init__ = original_init
|
||||
R._build_client = original_build
|
||||
|
||||
assert len(built) == 1, f"built {len(built)} repositories, expected 1"
|
||||
assert len(set(instances)) == 1, "the threads got different repositories"
|
||||
|
||||
|
||||
def test_the_reading_id_cannot_be_used_to_inject_a_query(repo):
|
||||
"""The reading id is coerced before it reaches Mongo.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user