Fix Elasticsearch index create race in tests

[kitcommerce]

Closes #650.\n\nExtracted from stacked PRs #630/#631 to make merge order linear.

[kitcommerce]

Dispatcher Build Gate Summary (local)

• rubocop (diff-only): PASS (0 offenses)
• brakeman: skipped (per repo build gate config; disabled)
• tests (affected engines): PASS
  • core: PASS

Note: test output still prints BSON Symbol deprecation warning (expected to be addressed by WA-NEW-010 / PR #635).

[kitcommerce]

Architecture Review

Verdict: APPROVE (with nits)

What looks good

• Fixes a real TOCTOU race by removing the unless exists? precheck and instead handling the “already exists” response from Elasticsearch.
• Change is nicely localized to Workarea::Elasticsearch::Index and keeps create!(force:) as the single place for creation semantics.
• Backwards-compat risk appears low; caller-facing behavior is effectively unchanged.
• Test coverage added is aligned with the reported flake and covers idempotency + force recreation behavior.

Nits / suggestions

• The rescue path currently depends on e.message.include?('resource_already_exists_exception'), which is brittle (coupled to the client gem’s exception message formatting).
  • If available, prefer inspecting structured error data (e.g., parsed e.response body / error.type) instead of substring matching.

No blocking architecture concerns.

[kitcommerce]

Security Review

Verdict: APPROVE

Summary

• The change improves idempotency around Elasticsearch index creation by removing a TOCTOU precheck and handling the expected “already exists” failure case.
• No new auth, input validation, or data exposure paths are introduced.

Note (low severity)

• The rescue condition uses e.message.include?("resource_already_exists_exception"), which is less robust than matching structured error data (e.g., response body / error type) if available.

Other notes

• delete(ignore: [404]) is benign and consistent with Elasticsearch Ruby client conventions.

[kitcommerce]

Rails / Ruby Conventions Review

Verdict: APPROVE (with nits)

What looks good

• Idiomatic fix for the TOCTOU race: attempt the create and treat “already exists” as success, rather than unless exists?.
• create!(force:) remains clear and consistent.
• Fully-qualified exception constant is a nice touch.
• Tests cover idempotency and force-reset behavior.

Nits / suggestions

• The guard e.message.include?('resource_already_exists_exception') is workable but brittle across ES client versions.
  • Prefer checking structured error details when available (e.g., e.response.dig('error','type') == 'resource_already_exists_exception') with a fallback to message matching.
  • If staying with message matching, match?(/resource_already_exists_exception/) is a bit cleaner/stricter.

No blocking concerns.

[kitcommerce]

Simplicity Review

Verdict: APPROVE (with nits)

Main nits (non-blocking)

1. Comment verbosity / duplication

• The production method comment is very long (reads like an incident report) and similar prose is repeated in the test file.
• Suggest condensing to a few lines describing the TOCTOU issue + new behavior and linking to issue Fix Elasticsearch index create race in tests #650 for full context. Let test names do most of the talking.

2. Exception matching clarity

• Current rescue path uses substring matching on the exception message.
• If feasible, prefer structured error inspection; otherwise consider tightening it (regex match?) or centralizing the check in a small helper for readability.

3. ignore: [404] vs ignore: 404

• The format change may be unnecessary churn unless required for a specific client/version. If not required, keep the prior form for less noise.

4. Test scope

• Some assertions may be broader than strictly needed for the regression. Consider trimming to the minimal idempotency + double-force-reset behaviors, keeping any searchability/doc-count assertions only if they’re reliably stable.

No blocking simplicity concerns.

[kitcommerce]

Rails Security Review

Verdict: APPROVE (low risk)

Summary

• This is an Elasticsearch index-creation idempotency/race fix; it does not introduce any Rails params/mass-assignment/injection surfaces.

Notes (non-blocking)

• The rescue condition suppresses only the “already exists” case via e.message.include?('resource_already_exists_exception'). This is fine from a security standpoint, but could be made more robust by matching structured error type from the ES response if available.

Quick sanity check

• Confirm Index#create! isn’t used in a context where any create failure must always raise (e.g., invalid settings/mappings). Other BadRequest errors should still raise as-is.

[kitcommerce]

Database / Data Integrity Review

Verdict: APPROVE (with notes) — overall low risk

Scope

Elasticsearch index lifecycle (primarily test-time), idempotency/concurrency semantics. No migrations/schema/model changes.

What changed

• Replaces TOCTOU (exists? then create) with an idempotent create + rescue of the specific “already exists” failure.
• Tweaks delete ignore param (404 → [404]).
• Adds regression tests for idempotency and force recreate behavior.

Notes / considerations (non-blocking)

• The “already exists” detection relies on e.message.include?('resource_already_exists_exception') (brittle across client versions). Prefer structured error inspection when available.
• In highly concurrent scenarios with force: true: if this call deletes, another actor creates, and then this call hits the “already exists” rescue, it will accept the other actor’s index definition (mappings/settings). If force is only used in tests/controlled contexts this is fine; otherwise it may not guarantee “my exact mappings/settings.”

Recommendations

• If feasible, match on structured ES error type.
• Confirm force: true is confined to test/controlled usage; strengthen if ever used with concurrent actors.

[kitcommerce]

Test Quality Review

Verdict: COMMENT (tests are helpful, but a couple tweaks would reduce flake risk)

What the new tests cover well

• create! idempotency: calling create! when the index already exists exercises the new rescue behavior (expects no exception).
• Force reset idempotency: calling the force recreation twice should not raise.
• Usability after reset: saving/querying a doc after a force reset shows the index is usable.

Gaps / improvements (non-blocking but recommended)

• The idempotency test currently only asserts “does not raise.” Consider also asserting the index is healthy/searchable after the rescue path (e.g., wait_for_health and/or a basic count/search) so it can’t pass for the wrong reason.

Flake risk (medium) and mitigation ideas

• Elasticsearch operations can be eventually consistent. Foo.save followed immediately by Foo.count may flake unless the suite auto-refreshes.
• After Foo.reset_indexes!, consider explicitly synchronizing before assertions:
  • Foo.current_index.wait_for_health (or equivalent)
  • and/or an explicit refresh (Foo.refresh / Foo.current_index.refresh) before count assertions.

Overall: coverage is directionally correct for the regression; adding health/refresh synchronization would make it more reliable in CI.

[kitcommerce]

Accessibility Review

Verdict: NO A11Y CONCERNS

This PR only changes backend Elasticsearch index creation logic and adds/adjusts tests. There are no UI/markup/content/interaction changes, so no accessibility impact is expected.

[kitcommerce]

Frontend Review

Verdict: APPROVE

No frontend impact. Changes are confined to Ruby Elasticsearch index management and tests:

• No JS/CSS/templates touched
• No browser/runtime behavior changes
• No new/changed client extension points

No frontend QA follow-up needed.

[kitcommerce]

Performance Review

Verdict: APPROVE (with nits)

Summary

• No meaningful performance regression expected.
• Tradeoff: when the index already exists, we now do a create attempt that returns a 400 (exception path) instead of doing exists?.
  • Slightly heavier on that path.
  • When the index does not exist, this can actually be a net improvement (skips an extra exists? round-trip).

Nit / suggestion

• If supported by the elasticsearch client/version here, consider using ignore: [400] on indices.create (or otherwise matching on structured error data) to avoid exception overhead and brittle message substring matching.

Overall risk: low; merge is reasonable.

[kitcommerce]

Documentation Review (informational)

Verdict: Informational-only

PR description

• Adequate but very minimal.
• Suggest adding 1–2 sentences describing the actual change:
  • Removes the exists? TOCTOU guard; attempts create and rescues the “already exists” error.
  • Why it matters: stabilizes CI/tests by avoiding intermittent 400→later 404 cascades.
• Optional: include a brief “observed failure mode” line for future archaeology.

Docs / comments in code

• The method-level comment in Index#create! clearly explains the race + failure cascade and justifies the rescue.
• Test comments are high-quality; if the project prefers terser commentary, they could be trimmed, but they’re fine as-is.

No blocking documentation concerns.

[kitcommerce]

✅ All Review Waves Passed

All review waves returned PASS/APPROVE or informational notes.

• Wave 1 (Foundation): ✅
• Wave 2 (Correctness): ✅
• Wave 3 (Quality): ✅
• Wave 4 (Documentation): ✅ (informational)

Labeled merge:ready.