Skip to content

fix(embeddings): repair taxonomy embeddings for every field type, not only text [ENG-2792] - #153

Open
xernobyl wants to merge 5 commits into
mainfrom
fix/ENG-2792_embedding-reconcile-eligibility
Open

xernobyl wants to merge 5 commits into
mainfrom
fix/ENG-2792_embedding-reconcile-eligibility

Conversation

@xernobyl

Copy link
Copy Markdown
Contributor

What does this PR do?

Ref ENG-2792 (https://linear.app/formbricks/issue/ENG-2792) — this is the incident slice only. The ticket stays open for the durable-enqueue, stale-embedding and operator-repair work.

On 22 October an EU directory stayed on "Preparing your feedback": 6,642 categorical records had used up their taxonomy-embedding attempts on provider connection resets, and nothing ever retried them. The cause is one rule kept in several places that had drifted apart:

Path Before After
Live taxonomy provider, readiness counts, taxonomy run input any record with text unchanged
Repair sweep (ListPendingTaxonomyEmbeddingIDs) text records only any record with text
Pending gauge (hub_enrichment_pending_records{taxonomy_embedding}) text records only any record with text
Failed-records gauge (hub_enrichment_failed_records{taxonomy_embedding}) text records only any record with text
backfill-embeddings -taxonomy own copy, ASCII-space trim shared rule

So a categorical record (matrix and multi-choice answers store the option label in value_text) was embedded live and counted toward readiness, but couldn't be repaired, and both gauges reported the stuck backlog as zero. Every path now selects through taxonomyEmbeddingEligibleTextSQL, and the comments say why there's deliberately no field_type predicate on it.

The backfill command also stops picking up tab- or NBSP-only records it would re-enqueue on every run. The taxonomy docs page said only text records take part; it now says what actually happens and when repair kicks in.

Tests added

  • TestTaxonomyEmbeddingRecoversAfterTransientExhaustion reproduces the incident against real Postgres and River. The live provider enqueues, the worker's final attempt fails on ECONNRESET, the cooldown passes, the sweep queues a repair, the repair embeds the record, and readiness shows full coverage. Text passes on main and categorical fails; both pass here.
  • TestTaxonomyEmbeddingEligibilityAgreesAcrossPaths reads every field_type from the DB enum (so a new type is covered without touching the test) and asserts live enqueue, repair, readiness, both gauges and the backfill list/count agree on a record with text and a blank one. Reverting any single fix makes it fail (checked for each).
  • TestListPendingTaxonomyEmbeddingIDs now runs every exclusion (active job, terminal marker, model and revision checks, cooldown, whitespace) for categorical as well as text records.
  • The gauge test that pinned the categorical exclusion now expects it to be counted.

Test hygiene fix (same area). The new tests and two neighbours (TestListPendingTaxonomyEmbeddingIDs, TestCountFailedRecordsAggregateIncludesOnlyCurrentTaxonomyEmbeddingFailures) closed their pool with defer before t.Cleanup ran. The cleanup silently hit a closed pool and up to 1,000 repair jobs leaked per run, which broke the next run's sweep. They now use t.Cleanup(db.Close), like the rest of tests/.

How should this be tested?

Automated (against a pgvector Postgres with goose and River migrations):

DATABASE_URL=postgres://postgres:postgres@127.0.0.1:5432/test_db?sslmode=disable \
  go test ./tests/ -run 'TestTaxonomyEmbedding|TestListPendingTaxonomyEmbeddingIDs|TestCountFailedRecordsAggregate|TestCountEnrichmentBacklogAggregateIfLeader' -count=1 -v

To see the bug, check out main and run the same tests with only tests/embedding_reconcile_recovery_test.go copied over: the categorical subtest fails at "reconciler must enqueue a repair".

Live smoke I ran. Real hub-api and hub-worker binaries against ai-test-server (OpenAI-compatible stub with runtime failure injection), a fresh database per scenario, EMBEDDING_MAX_ATTEMPTS=1 so the first failure is final. Branch vs main:

# Scenario Branch main
1 All 9 field types: provider down → records exhaust → provider back, no edit 27/27 recovered, readiness full, 1 provider call per record text 3/3, every other type 0/3
2 Cooldown (RETRY_AFTER=30s) nothing before 30s, all after categorical never
3 TARGET_DEPTH=5, 40 stranded max repair-queue depth 5, 40/40, 40 calls 0/40
4 Live jobs queued/running while sweeps run every 1s 0 repair jobs, 24 calls for 12 records same
5 Terminal markers terminal excluded; editing value_text re-embeds categorical never repaired
7 Tenant delete (sync) and purge (async) mid-repair no orphan embeddings or markers, control tenant healed control tenant 0/5
8 Worker SIGKILL mid-repair, rescue 20s 10/10, ≤1 duplicate call 0/10
10 Two workers sweeps not multiplied, ≤1 repair job per record, 20 calls 0/20
11 429 storm snoozed jobs not duplicated by repair, no failure markers same
12 50% failures for 20s, 60 records 60/60, 172 calls (≤4/record) 48/60
13 Records inserted with no event (lost enqueue) text and categorical repaired categorical never
14 Gauges over OTLP, 10 text + 10 categorical stranded pending 20, failed 20, both 0 after healing pending 10, failed 10, and "0" while categorical were still stuck
15 backfill-embeddings -taxonomy -count-only with tab/NBSP rows 2 6

The original incident repro (10 text + 10 categorical, provider down then back) goes from 10 stranded on main to 0 here.

Gates: make build, make lint (0 issues), make migrate-validate, and the full go test ./... twice back to back against the same DB (all 21 packages pass, 0 leaked jobs).

Not verified locally: pnpm check / pnpm build for the docs page (one prose sentence; the docs-build workflow on this PR covers it).

Risk and rollout

  • One-off drain on deploy. Every categorical record missing its taxonomy embedding becomes eligible at once. With prod's settings (target depth 100, every 300s) that's at most ~1,200 repairs an hour, so EU's 6,642 take ~5.5h, plus one provider call each. The pending and failed gauges jump by the same amount: that's the real backlog becoming visible, not a new one. No alerts reference either gauge today.
  • Sweep cost. With almost nothing to repair, the sweep now probes every eligible record instead of only text ones. At 200k records (60% categorical): ~70–80 ms → ~300–315 ms, and the planner drops the parallel plan. It runs once per sweep on the leader, under a 2-minute deadline. With a real backlog it's faster (~3 ms vs ~18–48 ms), because the LIMIT is reached sooner. The gauge query is unchanged (~55 ms; it already scans the table).
  • Fairness. Oldest-first ordering means a large old categorical backlog is drained before newer failures elsewhere. Parked with the other fairness work rather than changed here.
  • Security. Reviewed: no new egress (the live path already sends these records to the provider), no new logging, no tenant-boundary change. Repair jobs re-read the record by id and write under the tenant lock.
  • No migration, no new env var, no API change.

Found while testing, not changed here (pre-existing, parked for the rest of the ticket):

  • Clearing value_text on a non-text record leaves its old vectors behind.
  • Prod runs River's default 1h stuck-job rescue, so a worker killed mid-repair holds that record for up to an hour.
  • The taxonomy gauges only exist when translation is configured.

Checklist

Required

  • Filled out the "How to test" section in this PR
  • Read Repository Guidelines
  • Self-reviewed my own code
  • Commented on my code in hard-to-understand bits
  • Ran make build
  • Ran make tests (integration tests in tests/)
  • Ran make fmt and make lint; no new warnings
  • Removed debug prints / temporary logging
  • Merged the latest changes from main onto my branch with git pull origin main
  • If database schema changed: added migration in migrations/ with goose annotations and ran make migrate-validate (no schema change)

Appreciated

  • If API changed: added or updated OpenAPI spec and ran contract tests (no API change)
  • If API behavior changed: added request/response examples or Swagger UI screenshots to this PR (no API change)
  • Updated docs in docs/ if changes were necessary
  • Ran make tests-coverage for meaningful logic changes

xernobyl and others added 4 commits October 23, 2026 09:02
…ent exhaustion [ENG-2792]

Drives the live taxonomy provider, the worker's final attempt on a
connection reset, the reconcile sweep, and the repair job against a
real Postgres and River. Text records recover; categorical records the
live path accepts are never selected for repair.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…NG-2792]

The live taxonomy provider, readiness counts and taxonomy run input
select any record carrying text, including the option labels choice
and matrix expansion store on categorical rows. The repair sweep and
the backlog gauge additionally required field_type = 'text', so a
categorical record that exhausted its attempts on transient provider
errors was never repaired, and its directory never reached taxonomy
readiness. This stranded 6,642 records in the EU incident.

Drop the extra predicate so all five paths select through
taxonomyEmbeddingEligibleTextSQL, document it as the single rule, and
add a parity test over every field_type enum value. The gauge test
that pinned the categorical exclusion now expects it to be counted.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… rule [ENG-2792]

The failed-records gauge still gated its taxonomy_embedding arm on the
text-enrichment eligibility (field_type = 'text', value_text only), so a
categorical record that exhausted its attempts showed as pending but
never as failed. The backfill command kept its own taxonomy rule with a
bare btrim, selecting whitespace-only records it re-enqueues on every
run. Both now select through taxonomyEmbeddingEligibleTextSQL, and the
parity test covers the pending and failed-records gauges and the
backfill list and count for every field type.

The new tests and two neighbours closed their pool with defer before
t.Cleanup ran, so fixtures and up to 1,000 repair jobs leaked into the
shared test database per run and broke the next run's sweep. They now
register db.Close with t.Cleanup first.

The taxonomy page now states the full set of conditions for repair.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ENG-2792]

Run the repair-selection test's exclusions (active job, terminal marker,
model and revision checks, cooldown, whitespace) for categorical as well
as text records, and correct three comments and a docs line that still
described taxonomy embeddings as text-only.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@xernobyl
xernobyl marked this pull request as ready for review October 23, 2026 11:32
@coderabbitai

coderabbitai Bot commented Oct 23, 2026

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 1fcdbdd6-f499-4f4b-96c2-c1448bc0d64c
📥 Commits

Reviewing files that changed from the base of the PR and between 5f69443 and 2318ce8.

📒 Files selected for processing: 9
  • cmd/api/app.go
  • docs/src/content/docs/core-concepts/taxonomy.mdx
  • internal/repository/embeddings_repository.go
  • internal/repository/enrichment_status_repository.go
  • internal/repository/taxonomy_repository.go
  • tests/embedding_reconcile_recovery_test.go
  • tests/embedding_reconcile_test.go
  • tests/enrichment_failures_test.go
  • tests/enrichment_status_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

Taxonomy embedding eligibility now includes non-blank translated text, or non-blank source text when no translation is available, across field types. Pending selection, backfill, backlog counts, and failed-record aggregation use the shared eligibility rule. Integration tests compare eligibility across these paths and cover recovery after a transient provider failure and cooldown. The documentation describes field-type coverage and reconciliation retry conditions.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 2318c

Taxonomy embedding selection appears consistent across the reviewed paths, with no identified issue requiring a fix before merge.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 8 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title follows Conventional Commits (fix(embeddings): ...) and states the main change: taxonomy embedding repair now covers every field type, not only text. It matches the raw summary, which shows …
Description check Passed The description is thorough and follows the template. It explains the problem, the affected paths, and the fix. It lists automated test commands and a detailed live smoke matrix, and it covers rollout…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title follows Conventional Commits (fix(embeddings): ...) and states the main change: taxonomy embedding repair now covers every field type, not only text. It matches the raw summary, which shows eligibility widened from text-only to any record with taxonomy-eligible text across the repair sweep, gauges, and backfill. The ticket reference is an extra detail but does not reduce clarity.

Full details: Description check

Explanation

The description is thorough and follows the template. It explains the problem, the affected paths, and the fix. It lists automated test commands and a detailed live smoke matrix, and it covers rollout risks. The checklist is filled in. The template's placeholder 'Fixes #(issue)' was replaced with a Linear reference (Ref ENG-2792), and the migration item is marked unchecked with a note that no schema change exists. The 'make tests-coverage' item is unchecked without explanation, but the required items are addressed, so the description is mostly complete.

Full details: Docstring Coverage

Explanation

Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 8 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant