Repository navigation
[GH-1102] Clean invalid episode_ids during ingestion - #1148
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #1102 by making semantic ingestion resilient to dangling/invalid episode_id references in semantic storage, optionally failing loudly in debug mode.
Changes:
- Filter out missing episodes during ingestion and collect invalid
history_ids. - In non-debug mode, delete invalid
history_idsfrom semantic storage instead of raising. - Add error logging and a debug-only exception path for faster investigation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(none_h_ids) != 0: | ||
| raise ValueError( | ||
| "Failed to retrieve messages. Invalid episode_ids exist for set_id " | ||
| f"{set_id}: {none_h_ids}" | ||
| logger.error( | ||
| "Failed to retrieve messages. Invalid episode_ids exist for set_id %s, will delist following messages: %s", | ||
| set_id, | ||
| none_h_ids, | ||
| ) | ||
| if self._debug_fail_loudly: | ||
| raise ValueError( | ||
| "Failed to retrieve messages due to invalide episode_ids" | ||
| ) | ||
|
|
||
| try: | ||
| await self._semantic_storage.delete_history( | ||
| history_ids=none_h_ids, | ||
| ) | ||
| except Exception: |
There was a problem hiding this comment.
There are existing tests for IngestionService, but this new behavior (deleting invalid history_ids from semantic storage and the _debug_fail_loudly branch) isn’t covered. Adding a test that seeds a set with one valid and one missing episode_id and asserts delete_history() is called (and that debug mode raises) would prevent regressions.
| ) | ||
| if self._debug_fail_loudly: | ||
| raise ValueError( | ||
| "Failed to retrieve messages due to invalide episode_ids" |
There was a problem hiding this comment.
The debug-mode ValueError message has a typo ("invalide") and drops the most useful context (set_id and the invalid episode_ids). Consider fixing the spelling and including set_id + none_h_ids in the exception text so debug runs remain actionable.
| "Failed to retrieve messages due to invalide episode_ids" | |
| f"Failed to retrieve messages for set_id {set_id} due to invalid episode_ids: {none_h_ids}" |
| logger.error( | ||
| "Failed to retrieve messages. Invalid episode_ids exist for set_id %s, will delist following messages: %s", |
There was a problem hiding this comment.
This condition is handled by deleting the bad history references, so logging it at ERROR may be misleading operationally (it can trigger alerts despite the service recovering). Consider using WARNING (and/or clarifying the wording to indicate recovery) unless you intend to treat this as a hard failure.
| logger.error( | |
| "Failed to retrieve messages. Invalid episode_ids exist for set_id %s, will delist following messages: %s", | |
| logger.warning( | |
| "Failed to retrieve messages. Invalid episode_ids exist for set_id %s; delisting the following messages as recovery: %s", |
0a0e5ca to
6f480f1
Compare
…nt filter, add tests - Fix 'invalide' typo and include set_id + invalid IDs in ValueError message - Change logger.error to logger.warning since service recovers by deleting - Remove redundant raw_messages None filter (already filtered in comprehension) - Add tests for delete_history recovery path and debug_fail_loudly raise
6f480f1 to
c2cebf1
Compare
Purpose of the change
Gracefully handle invalid
episode_identries discovered during semantic ingestion instead of crashing the pipeline. This is a quick fix for #1102 while the root cause of the invalid IDs is still being investigated.Description
When
_process_single_setfetches episode history, someepisode_idreferences in the semantic store may point to episodes that no longer exist (returningNone). Previously this raised a hardValueError, halting the entire ingestion pipeline.This change:
Noneresults directly in the list comprehension, removing the redundant second filter.set_id.debug_fail_loudlypath that raises immediately in debug/test environments so the issue is still surfaced during development.Fixes/Closes
Related to #1102
Type of change
How Has This Been Tested?
Two new async pytest cases in
tests/memmachine/semantic_memory/test_semantic_ingestion.py:test_process_single_set_deletes_invalid_episode_ids— confirms invalid IDs are delisted and valid messages are still processed.test_process_single_set_raises_in_debug_mode_for_invalid_ids— confirmsValueErroris raised withdebug_fail_loudly=True.Checklist
Screenshots/Gifs
N/A
Further comments
This is an interim fix. The root cause of why invalid
episode_idreferences end up in the semantic store is still under investigation. Once resolved, the recovery/deletion logic here will act as a safety net rather than the primary handling path.