You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Split out of #2707 (review thread), where @goldmedal and I agreed it needs its own change because both fixes alter watch_loop's contract.
What happens
watch_loop treats every reindex failure as transient:
# core/wren/src/wren/memory/watch.py:172try:
poll_once(project_path, state, reindex, on_event=on_event)
exceptException:
# A transient reindex/poll failure must not kill the watcher.# poll_once keeps the old fingerprint on reindex failure, so# the same change is retried on the next interval.ifon_eventisnotNone:
on_event("error")
...
sleep(interval)
continue
poll_once (watch.py:124-130) emits reindex-error and re-raises, so this is the only place the exception is swallowed.
The comment's assumption is right for the failures it was written for — a half-written mdl.json, a locked table. It does not hold for a configuration error, which is identical on every retry.
Two consequences, and the second is the one that costs a user their afternoon:
A permanent failure is retried forever. Point WREN_EMBEDDING_MODEL at a CLS-pooled model, or at a repo with no ONNX export while running the onnx backend, and the watcher retries every interval until it is killed. The index never updates.
The reason is dropped.on_event receives only an event name, so the CLI (cli.py, _on_event) can print no more than:
Reindex failed; change kept pending, will retry next poll.
Meanwhile the exception it just discarded said, in full:
The onnx embedding backend implements mean pooling, but 'BAAI/bge-small-en-v1.5'
pools with ['cls_token']. Set WREN_EMBEDDING_BACKEND=sentence-transformers to use
this model.
Every other command shows that message — feat(wren): add a torch-free onnx embedding backend for wren memory #2707 routes OnnxBackendError through _MemoryGroup.invoke, so index, store, recall and fetch all print it and exit 1. watch is the one place a user cannot see why their index stopped updating, and it is the command they are most likely to leave running unattended.
Reproduce
export WREN_EMBEDDING_BACKEND=onnx
export WREN_EMBEDDING_MODEL=BAAI/bge-small-en-v1.5 # CLS-pooled
wren memory watch
# touch knowledge/sql/anything.md# -> "Reindex failed; change kept pending, will retry next poll." every interval, forever
Two independent fixes
Carry the reason.on_event takes an event name and nothing else. Widening it to accept the exception — or adding an on_error(event, exc) — lets _on_event print the message it already knows how to print. This alone turns an unexplained loop into a diagnosable one, and is the smaller change.
Stop retrying what cannot succeed.OnnxBackendError is by construction not transient: every subclass means "this backend cannot serve this model", which no amount of waiting changes. Letting that class propagate out of watch_loop (or counting consecutive identical failures and giving up) ends the loop with the message instead of hiding it. Worth deciding deliberately whether the terminal set is OnnxBackendError specifically or something broader — I lean specific, because anything wider risks killing a watcher over a genuinely transient error, which is the failure mode the current code was written to avoid.
They compose: the first makes the loop legible, the second makes it terminate. Either is an improvement on its own.
I'm happy to open a PR for both if that's useful — say which shape you'd prefer for the second one first, since that's the part with a real design choice in it.
Split out of #2707 (review thread), where @goldmedal and I agreed it needs its own change because both fixes alter
watch_loop's contract.What happens
watch_looptreats every reindex failure as transient:poll_once(watch.py:124-130) emitsreindex-errorand re-raises, so this is the only place the exception is swallowed.The comment's assumption is right for the failures it was written for — a half-written
mdl.json, a locked table. It does not hold for a configuration error, which is identical on every retry.Two consequences, and the second is the one that costs a user their afternoon:
A permanent failure is retried forever. Point
WREN_EMBEDDING_MODELat a CLS-pooled model, or at a repo with no ONNX export while running the onnx backend, and the watcher retries every interval until it is killed. The index never updates.The reason is dropped.
on_eventreceives only an event name, so the CLI (cli.py,_on_event) can print no more than:Meanwhile the exception it just discarded said, in full:
Every other command shows that message — feat(wren): add a torch-free onnx embedding backend for wren memory #2707 routes
OnnxBackendErrorthrough_MemoryGroup.invoke, soindex,store,recallandfetchall print it and exit 1.watchis the one place a user cannot see why their index stopped updating, and it is the command they are most likely to leave running unattended.Reproduce
Two independent fixes
Carry the reason.
on_eventtakes an event name and nothing else. Widening it to accept the exception — or adding anon_error(event, exc)— lets_on_eventprint the message it already knows how to print. This alone turns an unexplained loop into a diagnosable one, and is the smaller change.Stop retrying what cannot succeed.
OnnxBackendErroris by construction not transient: every subclass means "this backend cannot serve this model", which no amount of waiting changes. Letting that class propagate out ofwatch_loop(or counting consecutive identical failures and giving up) ends the loop with the message instead of hiding it. Worth deciding deliberately whether the terminal set isOnnxBackendErrorspecifically or something broader — I lean specific, because anything wider risks killing a watcher over a genuinely transient error, which is the failure mode the current code was written to avoid.They compose: the first makes the loop legible, the second makes it terminate. Either is an improvement on its own.
I'm happy to open a PR for both if that's useful — say which shape you'd prefer for the second one first, since that's the part with a real design choice in it.