Fix merge_message crash loop on nodes missing optional TRAPI fields - #153
Merged
Conversation
The merge_message worker was logging the same traceback dozens of times a
second, on repeat, for the same two queries:
File "/app/shepherd_utils/shared.py", line 984, in merge_kgraph
if value["name"]:
KeyError: 'name'
merge_kgraph subscripted `name`, `categories` and `attributes` on nodes and
`attributes`/`sources` on edges. All of those are optional in TRAPI, and a
subservice is free to omit one or send it as null -- `is_support_edge` right
above already guards for exactly that on `attributes`. So one node without a
`name` on an id already in the accumulator raised out of the process-pool
child and took down the entire batch merge. Read them with .get() instead.
That alone was a bad callback; what made it a crash loop is the failure path.
A failed merge re-enqueued its wake task immediately and unconditionally, and
re-enqueueing mints a new stream message, so Redis' `times_delivered` (what
the reclaim poison-pill breaker reads) resets to 1 every time and could never
trip. A deterministic failure therefore spun: merge, raise, re-enqueue, with
no backoff, for as long as the query lived -- burning a pool slot and never
finishing the query.
So the failure path now backs off exponentially between attempts, carrying
the attempt count in the task fields, and after merge_max_attempts (3)
consecutive failures discards the batch it cannot merge -- clearing it from
the ready index and callbacks table -- then re-enqueues with the counter
reset so the query merges everything else and finishes. BrokenProcessPool
goes through the same breaker: a batch that OOM-kills the child kills it
again on every retry.
_clear_batch moves to module level alongside the new handler so both are
unit-testable.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011Ms8RqVffNEzs3X1Qz15To
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
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.
The merge_message worker was logging the same traceback dozens of times a second, on repeat, for the same two queries:
KeyError: 'name'
merge_kgraph subscripted
name,categoriesandattributeson nodes andattributes/sourceson edges. All of those are optional in TRAPI, and a subservice is free to omit one or send it as null --is_support_edgeright above already guards for exactly that onattributes. So one node without anameon an id already in the accumulator raised out of the process-pool child and took down the entire batch merge. Read them with .get() instead.That alone was a bad callback; what made it a crash loop is the failure path. A failed merge re-enqueued its wake task immediately and unconditionally, and re-enqueueing mints a new stream message, so Redis'
times_delivered(what the reclaim poison-pill breaker reads) resets to 1 every time and could never trip. A deterministic failure therefore spun: merge, raise, re-enqueue, with no backoff, for as long as the query lived -- burning a pool slot and never finishing the query.So the failure path now backs off exponentially between attempts, carrying the attempt count in the task fields, and after merge_max_attempts (3) consecutive failures discards the batch it cannot merge -- clearing it from the ready index and callbacks table -- then re-enqueues with the counter reset so the query merges everything else and finishes. BrokenProcessPool goes through the same breaker: a batch that OOM-kills the child kills it again on every retry.
_clear_batch moves to module level alongside the new handler so both are unit-testable.
Claude-Session: https://claude.ai/code/session_011Ms8RqVffNEzs3X1Qz15To