fix: stop evicting paid verdicts on project-wide comparison_complete flips - #308
Merged
Conversation
Owner
Author
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Bundle ReportBundle size has no change ✅ Affected Assets, Files, and Routes:view changes for bundle: engram-frontend-client-array-pushAssets Changed:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #308 +/- ##
=======================================
Coverage 91.10% 91.11%
=======================================
Files 223 223
Lines 23320 23356 +36
=======================================
+ Hits 21246 21281 +35
- Misses 2074 2075 +1 ☔ View full report in Codecov by Harness. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Shortlist revalidation stops comparing
comparison_complete. Instead, completeness is checkedonly for the outcomes that actually read it, with a short bounded wait before the transaction and
a hard re-check inside it.
Why
comparison_completeis project-wide and flips constantly: every write outcome creates a newmemory version, therefore a new
RetrievalDocumentwith a NULLembedding_pgvector, andcorpus_fully_embeddedis false if any current document in the authorized corpus isunembedded. Because it sat in
shortlist_revalidation_hash, one publish anywhere in the projectevicted every other in-flight decision — after its provider call had already been paid for.
Measured on the stand: 648 paid
stale_decisionfrom_revalidate_shortlistover 9h; in thelatest 32-minute canary window, 65 stale against 33
judge_invalid_output. Paid stale is thelargest remaining source of discarded inference.
PR #306 removed
authorized_corpus_countfrom this same hash for the same reason and doubled theyield. It did not touch
comparison_complete. This finishes that job.How the gating stays honest
decision_requires_comparison_completedoes not hardcode a list of outcomes. It re-runsderive_decisionwithreplace(facts, comparison_complete=False)and reports whether the deriveddecision changes. If a rung's dependence on completeness ever changes, this follows automatically.
A lattice test pins the currently-gated set to exactly
{publish_new, supersede_memory, open_conflict}.This is exact rather than approximate because the verdict itself is produced by the same pure
derive_decisionover the samejudge_inputand the samecomparisons(
parse_curation_judge_verdict, curation_judge.py:478-481) — the helper re-derives, it does notguess.
Lowering
comparison_completeTrue→False is monotonic: it only removes eligibility, never addsit. So
derived != downgradedcatches exactly the dependence on completeness, andreject_candidatecan never require it — which is why the wait is deliberately not run on thereject path, leaving that path's lock ordering byte-for-byte unchanged.
The bounded wait
3 attempts, 2.0s apart → at most 4.0s of sleep and 3 read-only shortlist rebuilds.
_shortlist_memory_fenceis a plain read, and thetargetless-publication advisory lock is taken inside
transaction.atomic()after the wait.WORK_TIMEOUT_SPECS[CANDIDATE_DECISION]is soft 240 / hard 270 / lease 300; a work thatreaches the wait has spent ≤90s, so 4s is ~2.7% of the remaining budget. The wait cannot expire
a lease, including on the retry path.
_revalidate_shortlist, which raisesstale_decisionat once.What this does NOT fix, stated plainly
The canary outcome mix is 105 publish / 42 merge / 0 reject. Eviction used to be outcome-blind,
so roughly 71% of the paid stale was publish_new — the one outcome that still requires
completeness. The hash relaxation fully saves only merge/revise, about 29%.
For publish the saving depends on an assumption that has not been measured: that the embedding
lands inside the 4s window. The ~0.3s figure comes from PR #305 and was not re-measured here.
And structurally, the loser of two concurrent targetless publications still always gets stale —
its re-check runs immediately after the winner releases the lock, with no time for an embedding.
After deploy, measure residual paid stale for
publish_newspecifically, not the total.Known limitation: if the embedding queue stalls for longer than the wait, every
publish_newattempt burns 4s and then re-pays for the judge call on retry. That is pre-existing behaviour —
this change neither worsens nor protects against it. There is no circuit breaker.
Test changes, fully disclosed
Beyond the five re-authored tests described in the commits, this branch also sweeps
replace(orch.shortlist_entry(memory), team_id=None)across roughly 20 pre-existing tests incuration_tests.py(merge / revise / supersede / conflict / cp4-fault / concurrency /redundant-reject).
Reason: those fixtures encoded verdicts that are not derivable under the inverted contract.
CurationShortlistEntry.team_idcomes frommemory.team_id, andPromoteMemoryCandidatesetsteam_id = candidate.team_ideven forPROJECTvisibility, while the effective scope of aproject candidate is
(PROJECT, None)— sosame_visibilitywas False and every mutation rungwas unreachable for those fixtures.
This is a fixture artefact, not a production bug — verified on live data: all 14952
approved/conflict memories on the stand are
projectvisibility withteam_idNULL, sosame_visibilitybehaves correctly in production.Consequence worth stating: no test now exercises
_requires_comparison_completethrough ateam-bearing entry. Risk is low — in production the verdict and the facts are always produced by
the same
derive_decision— but the gap is real.Verification
pytest -m "not transactional"→ 3514 passed.pytest -m transactional→ 89 passed.engram_curator_eval→ all thresholds at bound, no threshold weakened.ruff check,ruff format --check→ clean.Tests were proven to bind by nine mutations, including: making the helper always return True,
making
_settle_revalidationunconditionally stale, and deleting the in-transaction guard —each producing a specific, named failure. Both hashes are pinned in opposite directions
(
test_revalidation_hash_ignores_comparison_completenessandtest_manifest_hash_still_covers_comparison_completeness).One unattributed flake was seen: a single
StopIterationin the transactional split, followed bysix green runs. Mechanism is plausibly unrelated to this diff but was not attributed — worth
remembering on a first red CI here.