Skip to content

fix: simplify embedding rate-limit re-enqueue and clean up tests#615

Merged
MaojiaSheng merged 1 commit intomainfrom
fix/embedding-rate-limit-requeue-and-test-cleanup
Mar 15, 2026
Merged

fix: simplify embedding rate-limit re-enqueue and clean up tests#615
MaojiaSheng merged 1 commit intomainfrom
fix/embedding-rate-limit-requeue-and-test-cleanup

Conversation

@qin-ctx
Copy link
Copy Markdown
Collaborator

@qin-ctx qin-ctx commented Mar 15, 2026 •
edited
Loading

Description

Simplify the 429 rate-limit error handling in TextEmbeddingHandler by moving re-enqueue logic directly into the exception handler, and clean up related tests.

Related Issue

N/A

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test update

Changes Made

  • Move 429 re-enqueue logic from a deferred flag-check pattern into the exception handler directly, avoiding unnecessary DB writes on rate-limit errors
  • Import is_429_error at module top level instead of inline
  • Add has_queue_manager property and enqueue_embedding_msg method to VikingVectorIndexBackend base class
  • Remove obsolete account_id parameter from get_context_by_uri calls in session commit tests
  • Remove outdated TestFindSimilarMemoriesURIConversion test class from deduplicator URI tests

Testing

  • New and existing unit tests pass locally with my changes
  • I have tested this on the following platforms:
    • Linux
    • macOS
    • Windows

Checklist

  • My code follows the project coding style
  • I have performed a self-review of my code
  • My changes generate no new warnings
  • Any dependent changes have been merged and published

Screenshots (if applicable)

N/A

Additional Notes

@qin-ctx @claude
fix: simplify embedding rate-limit re-enqueue and clean up tests
ccef05c 
Move 429 re-enqueue logic directly into the exception handler instead of
deferring via a flag, making the flow clearer and avoiding unnecessary DB
writes on rate-limit errors. Add has_queue_manager/enqueue_embedding_msg
to base backend class. Remove obsolete account_id param from test and
prune outdated URI deduplicator test cases.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 15, 2026 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@MaojiaSheng MaojiaSheng merged commit f442470 into main Mar 15, 2026
5 of 6 checks passed
@github-project-automation github-project-automation bot moved this from Backlog to Done in OpenViking project Mar 15, 2026
@MaojiaSheng MaojiaSheng deleted the fix/embedding-rate-limit-requeue-and-test-cleanup branch March 15, 2026 02:55
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 15, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Re-enqueue Logic

Verify that embedding_msg is properly defined in the scope where enqueue_embedding_msg is called, and that the re-enqueue flow correctly marks the original dequeue as successful when re-enqueued.

 
if is_429_error(e) and self._vikingdb.has_queue_manager:
    try:
        await self._vikingdb.enqueue_embedding_msg(embedding_msg)
        logger.info(f"Re-enqueued embedding message: {embedding_msg.id}")
        self.report_success()
        return None
Base Class Implementation

Confirm that subclasses of VikingVectorIndexBackend which set has_queue_manager to True also properly implement enqueue_embedding_msg to avoid NotImplementedError.

 
@property
def has_queue_manager(self) -> bool:
    return False

async def enqueue_embedding_msg(self, _embedding_msg) -> bool:
    raise NotImplementedError("Queue management requires VikingDBManager")

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 15, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Safely check for has_queue_manager attribute

Use getattr to safely check for the has_queue_manager attribute, preserving backward
compatibility and avoiding AttributeError if the attribute is not present. This
matches the original defensive pattern.

openviking/storage/collection_schemas.py [224]

-if is_429_error(e) and self._vikingdb.has_queue_manager:
+if is_429_error(e) and getattr(self._vikingdb, "has_queue_manager", False):
Suggestion importance[1-10]: 6

__

Why: Using getattr preserves backward compatibility by avoiding AttributeError if a _vikingdb implementation lacks the has_queue_manager attribute, matching the original defensive pattern. This is a reasonable safeguard even with the new backend additions.

Low

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MaojiaSheng MaojiaSheng MaojiaSheng approved these changes

Assignees

No one assigned

Labels

Review effort 2/5

Projects

OpenViking project
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@qin-ctx @CLAassistant @MaojiaSheng