Repository navigation
Say a failure once, and stop the kept reply retyping itself - #5135
Merged
Merged
Conversation
…commentary Two bugs in what just merged, both mine. A prompt whose own reply never arrived said nothing at all, if the exchange before it had left a failed reply behind. The check for "is there a reply to speak for this" looked at both neighbours, so the previous exchange's reply answered for the newer prompt and the question just asked failed in silence. The reply above a prompt counts as its own only when nothing precedes it, which is the case the second-precision ordering actually produces. A long partial re-typed itself from the middle. The saved reply is queued behind the letter-by-letter drain while the error that follows clears the buffer, so the drain kept running against an emptied buffer and the text the user had just watched appear started again from character N. It now finishes the drain and lands the saved reply before the buffer is cleared. The guard I added against showing a notice above still-typing text never fired: the error handler sets isLoading false in the same tick, so by the time a prompt turns :error the flag is already down. What it did do was hide an older failure's notice for the length of any new request. Gone. The test covering it passed a prompt in :error alongside isLoading true, which the store cannot produce, so it would have passed with the behaviour removed. The rest is what the reviews caught in the prose. The changelog said the panel still renders one generic error, which stopped being true when the panel was rewritten in the same PR. The adapter and codecov said the upstream Tesla fix was pending; it shipped in 1.21.1 three weeks ago, so the note now says what a bump would delete. DEPLOYMENT.md's arithmetic left out the ten-second buffer an operator would size against the grace period. A test named for passing through an unrecognised error type was sending one that is on the allowlist. The failure category said it was kept rather than broadcast when it is both, a comment about a deleted session was about a deleted message, and the reaper matched its worker by a string that would silently stop matching if the module were renamed. And the commentary. 186 comment lines out, 107 back: the essays are cut to the sentence that was actually load-bearing, and anything the code already said is gone.
Security Review ✅
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5135 +/- ##
=======================================
- Coverage 90.9% 90.8% -0.0%
=======================================
Files 420 420
Lines 20883 20883
=======================================
- Hits 18973 18967 -6
- Misses 1910 1916 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Both reviews caught the same regression, and it is the third time I have got this wrong by looking at the message next door. Reading both neighbours silenced a prompt whose own reply never arrived, when the exchange before it had left a failed reply behind. Reading only the one before, and only at the start of a session, showed the failure twice from the second exchange onward, whenever a reply sorted ahead of its own prompt. Each fix was correct for the ordering in front of me and wrong for the next one. The messages are ordered by a timestamp stored to the second, so a reply that dies in the same second as its prompt can land either side of it. Neighbours cannot answer this. Each failed reply now claims the failed prompt it answers, once, over the whole list: the nearest one before it, or after it when it sorted ahead. A prompt shows its own notice only when nothing claimed it, and Try again retries the prompt that was claimed, which also fixes a retry that re-ran the question from two exchanges back. Finishing the drain now happens only when something is waiting on it. message_error names any message, and the reaper reports on ones several exchanges back, so an unguarded flush could cut short a reply still arriving. Nothing covered the sequence this is all for. A test now streams a chunk, queues a saved partial behind the drain, and fires the error mid-drain, asserting the partial reaches the store. Removing the flush fails it; dropping the forward search in the pairing fails two more. The rest is the reviews on the prose. The tesla note folded two separate fixes into one release: the option pass-through landed in 1.19.0, the stream reason in 1.21.1, and #912 is the issue we filed rather than the PR that fixed it. It also said two callers where there is one, and codecov said "past 1.21.1" when the fix is in it. The allowlist comment blamed entry.py, which only ever emits INTERNAL_ERROR; the types it defends against come from the service modules, so it names those instead. The bootstrap cull took one line too many, since "request" against "idle" is the distinction the names do not give. And three @doc false attributes sat on private functions, where Elixir discards them.
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.
Description
Fixes two bugs that shipped in #5074, and cuts the commentary that PR added.
A failed exchange showed its notice twice, or not at all, depending on how the messages happened to load. Timestamps are stored to the second, so a reply that dies in the same second as its prompt sorts either side of it, and the code was reading neighbours. Each failed reply now claims the prompt it answers, once, across the whole list. That also fixes Try again re-running the question from two exchanges back.
A long partial re-typed itself from the middle after failing, because the saved reply was queued behind the letter-by-letter drain that the error then cleared.
The rest is prose that had drifted: the tesla note folded two upstream releases into one, and the allowlist comment named the wrong Apollo module. 186 comment lines out, 107 back.
Validation steps
APOLLO_ENDPOINTat something that goes quiet, setAPOLLO_IDLE_TIMEOUT_MS=5000, send a message and let it fail. Send a second and let that one fail too. Each failure is reported once, under the reply it belongs to.AI Usage
Please disclose whether you've used AI anywhere in this PR (it's cool, we just want to know!):
You can read more details in our Responsible AI Policy
Pre-submission checklist
/reviewwith Claude Code):owner,:admin,:editor,:viewer)