Image index service never unregisters its image callbacks on stop() #72

Summary

ImageIndexService.start() registers two callbacks (image_index_default.py:181-182):

invoker.services.images.on_changed(self._on_image_changed)
invoker.services.images.on_deleted(self._on_image_deleted)

stop() (:189) has no counterpart. After stopping, the callbacks stay attached to the ImageService, so every image created or deleted still enqueues work onto a queue with no consumer, and still adds names to the dedup set.

Observed

callbacks after 1st start: 1 1
worker alive after stop:   False
callbacks after stop:      1 1
queue size after stop + 500 image creations: 500     <- unbounded, no consumer
pending set size after stop: 500
model_id after stop: 'test-model-hash'   (base-class docstring says "None if the indexer is not running")
callbacks after 2nd start: 2 2                        <- double registration

Why the double registration matters

start()'s guard against starting twice (:135-139) tests self._worker.is_alive(), which is False after a clean stop(). So the guard does not cover a stop→start cycle, and the second start registers a second pair of callbacks — every image change then enqueues twice.

The class explicitly reasons about avoiding duplicate registration in that guard's comment, so the intent is clear; the check just doesn't cover this path.

Reachability

Low in production: ApiDependencies.initialize constructs a fresh ImageIndexService and a fresh ImageService per app instance, so a stop→start cycle against a surviving ImageService does not occur today. It is reachable in tests and in any future code that restarts the service — e.g. toggling image_index_enabled at runtime, which is the obvious next feature request.

Suggested direction

Unregister both callbacks in stop() (needs an off_changed/off_deleted on ImageService, or storing and removing the bound methods), drain _queue/_pending, and clear _model_id so model_id matches its documented contract. Alternatively, make the double-start guard independent of thread liveness.

Context

Found during adversarial review of PR #35, deferred as low severity while image_index_enabled defaults to False.