Repository navigation
OpenConceptLab/ocl_online#244 | split concepts/mappings index across shards and replicas and doing single refresh - #928
Conversation
…shards and replicas and doing single refresh
paynejd
left a comment
There was a problem hiding this comment.
Codex adversarial review, pass 1 (codex-cli 0.160.1, model_reasoning_effort=xhigh, read-only sandbox, commit 4390b82). Posted verbatim; local file paths rewritten as links to the reviewed commit.
Reviewed head 4390b821 against merge-base 526f115c, including the pinned django-elasticsearch-dsl 8.0 source. Findings below were verified by source inspection and in-memory fault simulations. No files were changed; I did not run live Elasticsearch or Django tests.
-
[P1] Rollback can delete the only surviving index after a successful swap. es_split_index.py:47
If Elasticsearch commits the alias swap and deletes the source, but the response times out, the exception handler calls
rollback(). That deletes the target—including its alias—then attempts to unblock the already-deleted source. I reproduced this sequence: both indices and the alias disappear. A metadata change can apply despite an acknowledgement timeout. Elasticsearch aliases APIFix: Reconcile the actual alias and concrete-index state after ambiguous failures. Never delete a target currently serving the alias. Treat the destructive swap as an irreversible commit boundary.
-
[P1] Alias rebuilds permanently corrupt the worker’s document index names. tasks.py:244
The pinned library’s
search_index.py:274assigns the timestamped name to the shared registryIndexobject and never restores it. Subsequent tasks in that prefork process write directly to that concrete index. Another worker’s rebuild can delete it; a second rebuild in the same process instead creates an alias named after the previous concrete index and deletes the backing index ofconcepts. I reproduced the originalconceptsalias disappearing on the second rebuild.Fix: Restore every shared index name in
finally, including failure paths, or isolate the management command in a separate process. Test two rebuilds followed by ordinary indexing in the same process. -
[P1] The new alias rebuild discards writes made during population. tasks.py:244
The library populates the replacement, swaps the alias, then deletes the old index without catching up concurrent changes. An edit after its document was copied still goes to the serving alias from another worker or gunicorn. The swap discards that edit. This includes synchronous source attribute propagation in
core/sources/signals.py:26. I reproduced an updated old-index document being replaced by its stale copied version.Fix: Coordinate all indexing writers, or capture and replay changes through a final cutover barrier.
--use-aliasalone does not provide write consistency. -
[P1] The split’s write block permanently loses indexing updates. es_split_index.py:39
PostgreSQL saves and indexing tasks continue while Elasticsearch rejects writes.
handle_savehas no retry for these failures. Batch indexing exhausts its default 150-second backoff, then fails; the split may wait an hour and can additionally wait indefinitely for operator input. Removing the block does not replay those updates, leaving search inconsistent with PostgreSQL.Fix: Establish a durable deferral/replay mechanism for every indexing writer and drain active writes before splitting. Obtain interactive confirmation before blocking. Use
indices.add_block(block='write')for its documented in-flight-write barrier. Index block behavior -
[P1] Failure handling can leave production write-blocked indefinitely. es_split_index.py:39, es_split_index.py:124
The initial block request is outside
try. If it applies but its response is lost, cleanup never runs. Separately, rollback deletes the target before clearing the block; a deletion timeout prevents the unblock attempt. Both cases reproduced.--timeoutextends only split and health requests; block, flush, refresh, counts, swap and cleanup retain the ordinary client timeout.Fix: Guard the initial mutation, reconcile uncertain outcomes, and attempt source unblocking independently of target cleanup. Apply an explicit timeout policy throughout and preserve both the original and cleanup errors.
-
[P1] The production runbook finishes with zero replicas and all primaries on one node. es_split_index.py:102
The earlier manual replica update protects the source only. The split explicitly resets the target to zero replicas and pins it to the source node; cutover deletes the protected source. Executing the PR body’s commands therefore leaves the original single-node failure risk. Expunge, unpinning and replica restoration are only printed afterward, with no completion checks. The matching document counts do not establish that deleted documents were expunged.
Fix: Make finalization explicit and mandatory: complete and verify expunge, remove allocation pinning, configure one replica, and wait for green with copies distributed across nodes. Prefer establishing that protection before deleting the serving source.
-
[P1] Disk preflight permits merge growth across Elasticsearch watermarks. es_split_index.py:88
It checks current utilization and one source-sized amount of free space independently. A 300-GiB node using 210 GiB with 90 GiB free passes for a 78-GiB source. Consuming the command’s budgeted 78 GiB during merge overlap would reach 96% utilization. Hard links retain old segment files until their remaining links are released, and pinning prevents relocation elsewhere. Elasticsearch’s default flood watermark then blocks affected indices. Split behavior, Disk watermarks
Fix: Check projected peak utilization against effective cluster watermarks, including merge workspace and concurrent growth. Recheck before merging and relocation.
-
[P1] The documented rebuild command deletes the migrated backing indices before population. README.md:85
The README still specifies
search_index --rebuild -f --parallelwithout--use-alias. In the pinned library,_delete()explicitly deletes alias-backed indices for this invocation, then recreates concrete indices under the original names. Following the documented command after migration removes the aliases and takes search down during population.Fix: Update the documented rebuild procedure and preferably guard or override destructive rebuilds of aliased indices.
-
[P2] AC 3 remains unmet for full population and rebuild jobs. tasks.py:246
These commands bypass the modified
BaseModelbatching functions. Pinnedsearch_index.py:153callsDocument.update(..., refresh=None);documents.py:251supplies the document’sauto_refresh=True, which reaches every bulk. There is no separate terminal refresh. This affects Celery tasks, README commands and AGENTS.md’s populate command.tools/backfill_expansion_url_per_expansion.py:108also retains per-bulkrefresh=True; mapped-code batches explicitly opt out of terminal refresh.Fix: Route batch population through
refresh=Falsebulks and one explicit refresh per completed index, before alias cutover. Cover raw bulk callers and define any intentional opt-outs. Adding the library’s--refreshflag would still refresh every bulk. -
[P2] A failed terminal refresh is reported as successful indexing. models.py:185
The new refresh method catches every exception and returns normally. A job requesting refresh can report zero failures while its writes remain unsearchable. Immediate verification callers such as
tools/backfill_expansion_url_per_expansion.py:137then observe stale counts; with periodic refresh disabled, that persists. The new test explicitly accepts this result.Fix: Propagate refresh failure for callers requesting searchable completion, or return an explicit visibility failure that those callers must handle.
The repository-wide search found no additional search_index invocations in scripts or CLAUDE.md. --populate works through aliases but retains the refresh problem above. CI’s --create calls are safe on fresh indices; the pinned library skips existing aliases.
Unset shard/replica variables preserve the previous 1/0 layout, so local development and fresh CI retain their existing behavior. A subsequent rebuild in an environment with unset variables would also recreate 1/0, undoing the migration’s layout. The encrypted companion values remain unverified. Six compacted concepts primaries average approximately 13 GB before deleted-document savings, below the stated 15–20 GB target; shard counts should follow measured live size. Splitting one primary into six is supported.
Resolving the master conflicts can change behavior. Preserve master’s INDEX_BATCH_SIZE and vectorization changes together with this PR’s terminal-refresh logic; taking master’s batching hunk restores per-bulk refresh, while taking the PR’s documents file wholesale drops release-level vectorization. The combined implementation also makes master’s ConceptVectors read stored vectors through the mutated index name, so alias rebuilds miss reusable vectors held only in the serving index and encode them again.
The tests cover mocked preflight checks and pre-swap failures, but omit committed-but-timed-out swaps, cleanup failures, concurrent writers, replica allocation, merge headroom and registry reuse. The rebuild tests mock call_command, concealing the library behavior responsible for findings 2, 3 and 9.
Verdict: needs rework.
paynejd
left a comment
There was a problem hiding this comment.
Claude review (commit 4390b82). Codex's pass 1 is the review above. This one adds one-click fixes for the findings we share.
Checked against the acceptance criteria in OpenConceptLab/ocl_online#244 and the pinned django-elasticsearch-dsl 8.0 source. Verdict: needs changes before merge or the prod run. Two P1s can delete the live concepts index. One acceptance criterion isn't met yet.
| Finding | Codex | Inline | |
|---|---|---|---|
| P1 | --use-alias renames the registry's Index objects and never renames them back. A later rebuild in the same indexing worker deletes the index behind the alias |
2 | tasks.py:243 (suggestion) |
| P1 | rollback() can run after the alias swap went through, and then deletes the target, which is the only copy |
1, 5 | es_split_index.py:122 (suggestion) |
| P2 | AC 3: search_index --populate/--rebuild still sends refresh=true with every bulk chunk |
9 | tasks.py:243 (same suggestion) |
| P2 | The disk check allows a split whose merges would pass the 90%/95% disk watermarks | 7 | es_split_index.py:88 (suggestion) |
| P2 | The write block doesn't drain in-flight writes; writes during the block fail and aren't replayed | 4, 5 | es_split_index.py:39 (suggestion) |
| P2 | Saves during a --use-alias rebuild go to the old index, which the swap deletes |
3 | tasks.py:244 |
| P2 | README still documents a plain --rebuild, which breaks concepts/mappings once they're aliases |
8 | below |
| P2 | The runbook as written ends with every new primary pinned to one node and no replicas | 6 | below |
Codex rates several of these P1. I'd call them P2, because each needs a particular operator step or timing. All of them should be fixed before the prod run either way. On Codex's #10, a refresh failure that's only logged has low impact: these indices keep the default 1 s refresh interval, so a failed final refresh only delays visibility.
Verified: I applied the suggestions to a copy of this branch. pylint is 10.00/10 on the three files, and ESSplitIndexCommandTest, TaskTest and BaseModelTest pass (92 tests). Two new tests fail on this branch and pass with the fixes:
test_rebuild_indexes_restores_the_index_names:'concepts-20261006000000000000' != 'concepts'test_does_not_roll_back_a_swap_that_went_through:Expected 'delete' to not have been called. Called 1 times.
The test changes are in the patch at the end.
README.md:85 (outside the diff). Without --use-alias, search_index --rebuild deletes the index behind an alias. It then skips create, because the name was in the alias list it read at the start, and populates a name that ES auto-creates with dynamic mappings: no analyzers, no dense_vector. Suggested line:
- `docker exec -it oclapi2-api-1 python manage.py search_index --rebuild -f --parallel --use-alias` -- for rebuild (new timestamped indexes, then the aliases are swapped to them) all indexes. Keep `--use-alias` once `concepts`/`mappings` are aliases.
Runbook (PR description)
- As written, the runbook ends right after the split: all new primaries pinned to the source's node, 0 replicas, deleted docs still there. The expunge, unpin and replica steps are only printed by the command. Add them to the runbook, each with its check: merges finished, green, copies spread across nodes.
- Step 2 adds a replica to the source before the split. That copies the whole index to another node, and the swap deletes it again, since the split target starts with 0 replicas. Add replicas after the expunge and the unpin, as the command's
Next:line does. - After the swap, re-index what changed while writes were blocked.
POST /indexes/resources/concepts/(andmappings) withfilter={"updated_at__gte": "<block start>"}does it today. - Disk numbers and node headroom for the split are on OpenConceptLab/ocl_online#244.
After rebasing onto #926 (conflicts in core/common/models.py and core/concepts/documents.py):
- Keep master's
INDEX_BATCH_SIZEand vectorization changes together with this PR's single refresh. Taking either side wholesale drops one of them. ConceptVectors.index_namereturnsConceptDocument._index._name. Until the first P1 is fixed, a rebuilt worker reads stored vectors from the renamed index. During a--use-aliasrebuild it reads the new, still-empty index, so every concept is re-encoded. Reading the alias (ConceptDocument.Index.name) instead would let a rebuild reuse the vectors already stored.
Test changes that go with the suggestions (validated: pylint 10.00, 92 tests OK)
diff --git a/core/common/tests.py b/core/common/tests.py
index 4be71179..e7eadf9f 100644
--- a/core/common/tests.py
+++ b/core/common/tests.py
@@ -2173,23 +2173,36 @@ class TaskTest(OCLTestCase):
def test_populate_indexes_with_app_names(self, call_command_mock):
populate_indexes(['concepts'])
call_command_mock.assert_called_once_with(
- 'search_index', '--populate', '-f', '--models', 'concepts', '--parallel')
+ 'search_index', '--populate', '-f', '--models', 'concepts', '--parallel', refresh=False)
@patch('core.common.tasks.call_command')
def test_populate_indexes_without_app_names(self, call_command_mock):
populate_indexes(None)
- call_command_mock.assert_called_once_with('search_index', '--populate', '-f', '--parallel')
+ call_command_mock.assert_called_once_with('search_index', '--populate', '-f', '--parallel', refresh=False)
@patch('core.common.tasks.call_command')
def test_rebuild_indexes_with_app_names(self, call_command_mock):
rebuild_indexes(['concepts'])
call_command_mock.assert_called_once_with(
- 'search_index', '--rebuild', '-f', '--models', 'concepts', '--parallel', '--use-alias')
+ 'search_index', '--rebuild', '-f', '--models', 'concepts', '--parallel', '--use-alias', refresh=False)
@patch('core.common.tasks.call_command')
def test_rebuild_indexes_without_app_names(self, call_command_mock):
rebuild_indexes(None)
- call_command_mock.assert_called_once_with('search_index', '--rebuild', '-f', '--parallel', '--use-alias')
+ call_command_mock.assert_called_once_with(
+ 'search_index', '--rebuild', '-f', '--parallel', '--use-alias', refresh=False)
+
+ @patch('core.common.tasks.call_command')
+ def test_rebuild_indexes_restores_the_index_names(self, call_command_mock):
+ def rename(*_, **__): # as django_elasticsearch_dsl's _rebuild does with --use-alias
+ ConceptDocument._index._name = 'concepts-20261006000000000000' # pylint: disable=protected-access
+ raise CommandError('populate failed')
+
+ call_command_mock.side_effect = rename
+ with self.assertRaisesMessage(CommandError, 'populate failed'):
+ rebuild_indexes(['concepts'])
+
+ self.assertEqual(ConceptDocument._index._name, 'concepts') # pylint: disable=protected-access
@patch('core.importers.importer.Importer.run')
def test_bulk_import_new(self, run_mock):
@@ -3191,8 +3204,9 @@ class ESSplitIndexCommandTest(OCLTestCase):
client.cluster.health.return_value = {'status': 'green', 'timed_out': False}
client.cat.shards.return_value = [{'prirep': 'p', 'node': 'es2', 'store': str(10 * 1024 ** 3)}]
client.cat.allocation.return_value = [
- {'node': 'es', 'disk.percent': '10', 'disk.avail': '0'},
- {'node': 'es2', 'disk.percent': '45', 'disk.avail': str(100 * 1024 ** 3)},
+ {'node': 'es', 'disk.percent': '0', 'disk.used': '0', 'disk.avail': '0', 'disk.total': str(10 * 1024 ** 3)},
+ {'node': 'es2', 'disk.percent': '45', 'disk.used': str(45 * 1024 ** 3), 'disk.avail': str(55 * 1024 ** 3),
+ 'disk.total': str(100 * 1024 ** 3)},
]
client.count.return_value = {'count': 42}
for attr, value in overrides.items():
@@ -3212,7 +3226,8 @@ class ESSplitIndexCommandTest(OCLTestCase):
self.run_command(client, '--shards', '6', '--yes')
- client.indices.put_settings.assert_called_once_with(index='concepts', settings={'index.blocks.write': True})
+ client.indices.add_block.assert_called_once_with(index='concepts', block='write')
+ client.indices.put_settings.assert_not_called()
client.indices.flush.assert_called_once_with(index='concepts')
split_kwargs = client.indices.split.call_args[1]
target = split_kwargs['target']
@@ -3232,7 +3247,7 @@ class ESSplitIndexCommandTest(OCLTestCase):
self.run_command(client, '--shards', '6', '--dry-run')
- client.indices.put_settings.assert_not_called()
+ client.indices.add_block.assert_not_called()
client.indices.split.assert_not_called()
client.indices.update_aliases.assert_not_called()
@@ -3245,16 +3260,15 @@ class ESSplitIndexCommandTest(OCLTestCase):
({'indices.get_settings.return_value': {'concepts': {'settings': {'index': {
'number_of_shards': '1', 'blocks': {'read_only_allow_delete': 'true'}}}}}}, 'blocks set'),
({'cluster.health.return_value': {'status': 'red'}}, 'not green'),
- ({'cat.allocation.return_value': [
- {'node': 'es2', 'disk.percent': '90', 'disk.avail': str(100 * 1024 ** 3)}]}, 'above 85%'),
- ({'cat.allocation.return_value': [
- {'node': 'es2', 'disk.percent': '50', 'disk.avail': str(1024 ** 3)}]}, 'GB free'),
+ ({'cat.allocation.return_value': [ # 76 + 10 GB of 100: past 85% while the shards merge apart
+ {'node': 'es2', 'disk.used': str(76 * 1024 ** 3), 'disk.total': str(100 * 1024 ** 3)}]},
+ 'could reach 86% disk'),
]
for overrides, message in cases:
client = self.get_client(**overrides)
with self.assertRaisesMessage(CommandError, message):
self.run_command(client, '--shards', '6', '--yes')
- client.indices.put_settings.assert_not_called()
+ client.indices.add_block.assert_not_called()
with self.assertRaisesMessage(CommandError, 'at least 2'):
self.run_command(self.get_client(), '--shards', '1')
@@ -3287,6 +3301,17 @@ class ESSplitIndexCommandTest(OCLTestCase):
self.assert_rolled_back(client)
+ def test_does_not_roll_back_a_swap_that_went_through(self):
+ client = self.get_client()
+ # ES applied the swap, but its response was lost
+ client.indices.update_aliases.side_effect = Exception('ConnectionTimeout')
+ client.indices.exists_alias.side_effect = lambda name, index=None: index is not None
+
+ with self.assertRaisesMessage(CommandError, 'ConnectionTimeout'):
+ self.run_command(client, '--shards', '6', '--yes')
+
+ client.indices.delete.assert_not_called()
+
@patch('builtins.input', return_value='n')
def test_rolls_back_when_swap_is_declined(self, _):
client = self.get_client()Resolved conflicts with #926: kept INDEX_BATCH_SIZE with the single refresh per batch run, and master's settings import in concepts documents. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Summary of review followup Review follow-upThanks for both reviews (Codex pass 1 and Claude). master is merged in. Status of each finding:
Verified:
|
|
OCL Online docs PR -- https://github.com/OpenConceptLab/ocl-online-docs/pull/6 |
paynejd
left a comment
There was a problem hiding this comment.
Codex adversarial review, pass 2 (codex-cli 0.160.1, model_reasoning_effort=xhigh, read-only sandbox, commit d085d05). Follow-up on pass 1 and @snyaggarwal's table above; asked to verify fixes and lean toward done. Posted verbatim; local file paths rewritten as links to the reviewed commit. The runbook PR (ocl-online-docs#6) appeared after this ran, so Codex judged the PR description as the runbook; #6 gets its own pass.
Most code fixes are sound. #928 still needs the operational runbook corrected and the terminal-refresh gap closed. References below are at d085d051.
| Earlier finding | Status | Evidence |
|---|---|---|
| 1. Rollback deletes a successfully swapped target | Fixed | es_split_index.py:57 reconciles the alias and preserves serving or uncertain targets. |
| 2. Registry index names remain renamed | Fixed | tasks.py:255 restores every name in finally. |
| 3. Alias rebuild discards concurrent writes | Not fixed | tasks.py:252 still invokes population/swap without writer coordination or catch-up. |
| 4. Split block loses indexing updates | Partially fixed | es_split_index.py:47 drains writes; line 64 only prints a replay hint. |
| 5. Cleanup leaves writes blocked | Fixed | es_split_index.py:146 attempts unblocking independently; mutations are guarded and ordinary requests have a 120-second timeout. |
| 6. Runbook ends pinned, without replicas or expunge verification | Not fixed | es_split_index.py:109 creates zero replicas; finalization remains printed instructions. |
| 7. Disk preflight ignores projected growth | Fixed | es_split_index.py:96 checks used space plus source size against the threshold. |
| 8. README rebuild destroys aliases | Fixed | README.md:85 now specifies --use-alias. |
| 9. Population/rebuild refreshes every bulk | Partially fixed | tasks.py:254 passes refresh=False, but adds no terminal refresh; README commands retain per-bulk refreshes. |
| 10. Terminal refresh failures only logged | Declined (agree) | models.py:187: acceptable with the retained default periodic refresh. |
| Backfill script refreshes every bulk | Declined (agree) | backfill_expansion_url_per_expansion.py:108 supports its immediate verification count. |
| Vector reuse reads renamed index | Fixed | embeddings.py:129 reads the stable alias. Master’s batching and vectorization changes survive the merge. |
The runbook of record is insufficient to run safely: it still adds replicas before splitting and omits pause/drain/replay, expunge checks, unpinning, and replica/allocation verification.
For earlier findings 3/4, pausing indexing alone is insufficient: source permission propagation writes synchronously and leaves child timestamps unchanged. A permission change during the window can leave stale search permissions that updated_at__gte replay misses. Document controls for these writers and their recovery.
For finding 9, route management-command population through suppressed bulk refreshes plus one explicit refresh per completed index, before alias cutover.
No new findings.
oclinfrastructure#17: verdict posted on that PR.
Verification used read-only source inspection, 11 existing split tests executed with dependency stubs, and in-memory registry/library checks. No live Django/ES tests or file changes.
- oclapi2#928: Ready to merge after: complete the operational runbook; close the population/rebuild terminal-refresh gap.
- oclinfrastructure#17: Ready to merge.
|
Claude triage of Codex pass 2: the code is done. Both P1s and the other fixes check out, and Codex found nothing new in the code.
|
Linked Issue
Refs OpenConceptLab/ocl_online#244
one off tasks:
PUT concepts,mappings/_settings {"index.number_of_replicas": 1}-- manualpython manage.py es_split_index concepts --shards 6 --dry-runpython manage.py es_split_index mappings --shards 2 --dry-runpython manage.py es_split_index concepts --shards 6 --yespython manage.py es_split_index mappings --shards 2 --yesoclinfrastructure PR: https://github.com/OpenConceptLab/oclinfrastructure/pull/17
ocl-online-docs PR: https://github.com/OpenConceptLab/ocl-online-docs/pull/6