fix(indexing): treat shutdown-closed store during backfill as benign; de-flake reindex test - #1476
Conversation
There was a problem hiding this comment.
Code Review
This pull request improves the resilience of vector indexing during worker shutdowns by treating closed stores as benign interruptions rather than errors, and introduces a robust vectorSearchStable helper to handle transient indexing states in integration tests. The review feedback suggests minor improvements: simplifying the sleep utility in tests by using an existing import, and preserving error context in debug logs when logging interrupted indexing operations.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Reviewed; no blockers found. |
… de-flake reindex test
When `restart_service http_workers` tears a worker down mid-backfill, runIndexing's
range scan/puts throw against the closing store ("Database not open"). The old catch
logged a misleading error and then tried to persist `indexingFailed` against the
already-closed store, which also failed ("Failed to persist indexing failure state").
Treat a store closed by shutdown (`primaryStore.rootStore.status === 'closed'`) as a
benign interruption: skip the doomed persist and log at debug. Recovery is unchanged —
the next worker generation re-runs the backfill via the existing crash-recovery trigger
(indexingPID !== process.pid / restartNumber < current generation).
Also de-flakes the reindex integration test: a recovery-reindex window after the
restart-interrupted first backfill legitimately returns a transient INDEX_REBUILDING
(503), which the test's single-success gate didn't tolerate. A new vectorSearchStable()
polls past the transient window; a permanently-stuck index (never recovers) still fails.
Adds a unit test for the shutdown-interruption path (runIndexing resolves, no indexingFailed).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… in test Addresses Gemini review: pass the underlying error to logger.debug on the shutdown-interruption paths so the root cause (e.g. "Database not open") is captured; use the already-imported sleep() helper in vectorSearchStable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
c6e6baa to
3671b81
Compare
…durable-session test `subscribe with QoS=1 and reconnect with non-clean session` sequenced connect/ disconnect/publish steps with hard-coded `delay(10)`/`delay(50)` calls instead of waiting for the actual server-side state to settle. `client.endAsync()` only resolves once the local socket closes — it does not wait for the broker to finish tearing down the durable session — so reconnecting with the same clientId immediately after could race the broker (issue #1138 flagged this file as the #2-worst offender for this pattern). Under CI load the delays were occasionally too short, causing the intermittent "got 0 messages"/mocha timeout flakes seen on #410 and #1476. Replace the disconnect-then-reconnect delays with a wait on the broker's own `disconnected` event (already exposed via `server.mqtt.events`) for the matching clientId, and replace the pre-disconnect delay with a wait for the client's outgoing PUBACK. Also await the first offline publish (it was fire-and-forget while the other two were awaited), so all three publishes are durably committed before the reconnecting client asserts on redelivery. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
Fixes the ~50% CI flake on
integrationTests/server/vector-index-integrity.test.ts("reindex backfill") and removes misleading error noise during worker shutdown.Root cause (investigated with instrumentation + local repro)
When
restart_service http_workerstears a worker down mid-backfill,runIndexing's range scan/puts throw against the closing store (Database not open). The new worker generation re-runs the backfill via the existing crash-recovery trigger (indexingPID !== process.pid/restartNumber < currentRestartGeneration). During that recovery-reindex window the index legitimately reportsisIndexing=true, so searches transiently return503 INDEX_REBUILDING. The test's step-4 gate waited for a single successful search, then step 5 fired searches with no retry — a transient 503 in that window failed the test.Instrumented/amplified local runs confirmed the index always recovers (no permanent stuck state); the failure is the transient 503 plus benign-but-misleading teardown logging (
Error in indexing+Failed to persist indexing failure state).Changes
resources/databases.ts— inrunIndexing's outer catch and per-attribute error catch, treat a store closed by shutdown (primaryStore.rootStore?.status === 'closed') as a benign interruption: early-return without the doomedindexingFailedpersist, log atdebug. Recovery behavior is unchanged (handled by the pid/restartNumber trigger).integrationTests/server/vector-index-integrity.test.ts—vectorSearchStable()retries a transientINDEX_REBUILDING(503) up to 30s; used in steps 5/6. A permanently-stuck index (never clears) still surfaces the 503 and fails.unitTests/resources/indexRestartNumber.test.js— new test for the shutdown-interruption path (runIndexing resolves;indexingFailednot set).Where to look / what to verify
status === 'closed'early-return is gated so it only skips a persist that could not have succeeded anyway (closed store). On LMDB,dbisDBshares the env withprimaryStore, so the priordbisDB.putalready failed there too — no recovery regression. On RocksDB (CI),resetDatabases()never closes the store, soclosedonly occurs on real worker teardown → recovery via a new generation.Cross-model review (DLC step 10 — thorough)
Codex + Gemini + Harper domain pass. No open blockers. Two findings surfaced, both confirmed pre-existing / out of scope, not introduced here:
resetDatabases()mid-backfill can leave an index stuck — this predates the change (the old persist already failed against the same closed env; recovery there doesn't depend onindexingFailed). LMDB is not the indexing/replication target path. Flagged for awareness, not fixed here.putunhandled rejection for multi-value attributes — already documented in-code as pre-existing and out of scope.Generated by an LLM (Claude Opus 4.8).