Skip to content

Fix terminal embedding failure quarantine - #33

Open
blast-hardcheese wants to merge 5 commits into
mainfrom
fix/issue-32-embedding-quarantine
Open

blast-hardcheese wants to merge 5 commits into
mainfrom
fix/issue-32-embedding-quarantine

Conversation

@blast-hardcheese

Copy link
Copy Markdown
Collaborator

Summary

  • classify provider context-limit responses as deterministic input failures
  • atomically move terminal jobs from the legacy queue into versioned local quarantine metadata
  • expose quarantine, drain completion, and vector coverage in kin embed status

Fixes #32.

Test plan

  • python -m pytest -q tests/test_embedding_quarantine.py tests/test_embed_queue.py tests/test_vectors.py tests/test_cron_reliability.py tests/test_tasks.py tests/test_edit_store.py (205 passed)
  • python -m pytest -q reached an unrelated existing supervisor-acceptance failure: its isolated test shell cannot find direnv (282 passed, 1 skipped before failure).

Risk / rollback

  • No schema migration: quarantine is a versioned meta payload and older builds ignore it. Revert the commit to resume prior queue-only behavior.

@adaptcom adaptcom Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confidence Score: 1/5

Summary

Adds terminal-error quarantine but breaks synchronous indexing, quarantine cleanup, and readiness reporting; contextual cron also retries rejected inputs. Targeted tests pass; two full-suite failures also reproduce on base because crontab is missing.

Important Files Changed

File Overview
README.md Documents terminal quarantine and readiness distinction
src/kindex/cli.py Adds status counts but omits actionable quarantine diagnostics
src/kindex/vectors.py Adds quarantine with indexing, lifecycle, retry, and readiness defects
tests/test_embed_queue.py Updates assertions for expanded drain results
tests/test_embedding_quarantine.py Covers basic quarantine paths but misses reproduced lifecycle and caller regressions

Re-run review · View in Adapt

Comment thread src/kindex/vectors.py Outdated
Comment thread src/kindex/vectors.py
return {"status": "empty", "embedded": 0, "pending": 0}
return {"status": "empty", "embedded": 0, "pending": 0,
"quarantined": len(quarantine), "drain_complete": True,
"coverage_complete": not quarantine}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove quarantine records after successful reindex, deletion, or supersession; otherwise resolved nodes permanently keep coverage_complete false.

Comment thread src/kindex/vectors.py Outdated
Comment thread src/kindex/vectors.py Outdated
Comment thread src/kindex/cli.py
print(f"Indexed nodes: {result.get('indexed_nodes')}")
print(f"Vector rows: {result.get('vector_rows')}")
print(f"Queue pending: {result['queue_pending']}")
print(f"Quarantined: {result.get('quarantined', 0)}")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expose quarantined node IDs and sanitized failure reasons; both human and JSON status currently show only counts, leaving operators unable to identify rejected nodes.

Comment thread src/kindex/vectors.py Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

OpenAI embeddings permanently requeue nodes over the model context limit

1 participant