From 4d41d1617e191ff3c8b5136b8cbef9ae72d14222 Mon Sep 17 00:00:00 2001 From: pinokio Date: Thu, 15 Oct 2026 21:37:20 +0800 Subject: [PATCH] serve: a tool's picture starts the elastic encoder, and its turn counts _note_unreadable_tool_images calls vision.encode() without _vision_up(): once the elastic idle sweep stopped the encoder (elastic_idle_s without a picture - Vision.unload() nulls the proc, the cache on disk stays), the next picture a tool returned took the turn and wrote to a pipe that was no longer there. The AttributeError on the None proc escapes the (ValueError, OSError) filter and the route's shape filter (:5551), so the request dies as a 400 "malformed" instead of the note this method exists to write - and because clients re-send the history, every later request of the conversation dies the same way at this method's own line, before the user-picture encode below could start the encoder: the conversation stays broken until some other conversation's picture (or a restart) starts the encoder. The fix mirrors what prepare() does for its own pictures: the encoder starts inside the turn (_vision_up, the caller holds the FIFO), and the turn's end updates last_image_at - without it the sweep judges the just-started encoder by the previous picture's timestamp and reclaims it at once, and a tool-picture-only conversation never moves the timer at all. start()'s own failure (RuntimeError, the three raises around the READY read) joins the filter: it degrades to the documented note, it is not a reason to kill the request. Checked against the code: encode()'s own pipe death already becomes ValueError/OSError internally (the #1317 path raises ValueError, a write to a dead pipe is OSError), so the widened tuple matches what can still raise; ClientGone (a bare Exception, :312) passes through untouched; _vision_up starts what it found down, so the None-proc AttributeError cannot happen after it. Two cases in serve/test_vision_elastic.py pin the behavior; on the base tree both fail at the fake's "a picture reached an encoder that was not started" assert - the bug's exact signature, and a real Vision with proc=None raises the AttributeError verbatim; the file passes 9/9, and test_vision_busy / test_images with it - 32 OK. Not measured against a live encoder. No native files touched. --- serve/server.py | 8 ++++++-- serve/test_vision_elastic.py | 22 ++++++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/serve/server.py b/serve/server.py index 5a119ec7cf..3490886455 100644 --- a/serve/server.py +++ b/serve/server.py @@ -4296,8 +4296,12 @@ def _note_unreadable_tool_images(self, messages, cancel=None): if src.startswith(("http://", "https://")): src = fetched[item["source"]] = Vision.download(src) # outside the FIFO, as in prepare() with self._turn(cancel): # the encoder takes its turn with the requests (see below) - self.vision.encode(src) - except (ValueError, OSError) as e: + self._vision_up() # the elastic encoder starts with its picture, as prepare() does: it + try: # had stopped after elastic_idle_s, and encode died on the missing pipe + self.vision.encode(src) + finally: # as prepare() does: the idle timer counts this turn too, so the + self.last_image_at = time.time() # sweep does not reclaim the encoder this picture started + except (ValueError, OSError, RuntimeError) as e: why = str(e) if why is not None: print(f"[strata] a picture a tool returned was left out of the prompt: {why}", flush=True) diff --git a/serve/test_vision_elastic.py b/serve/test_vision_elastic.py index b8516b1a24..efb469ee97 100644 --- a/serve/test_vision_elastic.py +++ b/serve/test_vision_elastic.py @@ -118,6 +118,28 @@ def test_the_config_keys(self): self.assertFalse(v.elastic) self.assertEqual((v.elastic_mib, v.elastic_idle_s), (2048, 120.0)) + def tool_picture(self): + return [{"role": "user", "content": "read it"}, + {"role": "tool", "tool_call_id": "t1", "content": [dict(IMAGE)]}] + + def test_a_tool_picture_starts_an_idle_encoder_and_holds_it(self): + msg = self.tool_picture() + self.svc._note_unreadable_tool_images(msg) + self.assertEqual(self.vision.starts, 1) # the picture woke the encoder (it was down: no picture turn + self.assertTrue(self.vision.up) # ran yet, and encode died on the missing pipe) + self.assertGreater(self.svc.last_image_at, 0) # the turn counted for the idle timer, as prepare's does + self.assertEqual(self.svc._vision_idle_down(), "busy") # so the sweep leaves the new encoder alone + self.assertEqual(msg[1]["content"][0]["type"], "image") # and the picture stays in the history + + def test_a_tool_picture_whose_encoder_cannot_start_becomes_a_note(self): + def fail(): + raise RuntimeError("the vision encoder did not start") # what _vision_up's start raises + self.vision.restart = fail + msg = self.tool_picture() + self.svc._note_unreadable_tool_images(msg) # the start's RuntimeError must not escape: the method's + self.assertEqual(msg[1]["content"][0], # contract is a note in the picture's place, not a 500 + {"type": "text", "text": "[image omitted: the vision encoder did not start]"}) + if __name__ == "__main__": unittest.main()