Skip to content

OpenConceptLab/ocl_online#244 | split concepts/mappings index across shards and replicas and doing single refresh - #928

Merged
snyaggarwal merged 4 commits into
masterfrom
ocl_online#244
Oct 7, 2026
Merged

snyaggarwal merged 4 commits into
masterfrom
ocl_online#244

Conversation

@snyaggarwal

@snyaggarwal snyaggarwal commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Linked Issue

Refs OpenConceptLab/ocl_online#244

one off tasks:

  • grow the es and es3 disks first OpenConceptLab/ocl_online#243
  • update replicas -- PUT concepts,mappings/_settings {"index.number_of_replicas": 1} -- manual
  • split shards
    • dry-run:
      python manage.py es_split_index concepts --shards 6 --dry-run
      python manage.py es_split_index mappings --shards 2 --dry-run
    • actual run
      python manage.py es_split_index concepts --shards 6 --yes
      python manage.py es_split_index mappings --shards 2 --yes

oclinfrastructure PR: https://github.com/OpenConceptLab/oclinfrastructure/pull/17
ocl-online-docs PR: https://github.com/OpenConceptLab/ocl-online-docs/pull/6

…shards and replicas and doing single refresh
@snyaggarwal
snyaggarwal requested a review from paynejd October 5, 2026 11:15
@snyaggarwal snyaggarwal self-assigned this Oct 5, 2026
@snyaggarwal
snyaggarwal marked this pull request as ready for review October 5, 2026 11:21

@paynejd paynejd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

  1. [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 API

    Fix: 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.

  2. [P1] Alias rebuilds permanently corrupt the worker’s document index names. tasks.py:244

    The pinned library’s search_index.py:274 assigns the timestamped name to the shared registry Index object 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 of concepts. I reproduced the original concepts alias 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.

  3. [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-alias alone does not provide write consistency.

  4. [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_save has 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

  5. [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. --timeout extends 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.

  6. [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.

  7. [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.

  8. [P1] The documented rebuild command deletes the migrated backing indices before population. README.md:85

    The README still specifies search_index --rebuild -f --parallel without --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.

  9. [P2] AC 3 remains unmet for full population and rebuild jobs. tasks.py:246

    These commands bypass the modified BaseModel batching functions. Pinned search_index.py:153 calls Document.update(..., refresh=None); documents.py:251 supplies the document’s auto_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:108 also retains per-bulk refresh=True; mapped-code batches explicitly opt out of terminal refresh.

    Fix: Route batch population through refresh=False bulks and one explicit refresh per completed index, before alias cutover. Cover raw bulk callers and define any intentional opt-outs. Adding the library’s --refresh flag would still refresh every bulk.

  10. [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:137 then 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 paynejd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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/ (and mappings) with filter={"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_SIZE and vectorization changes together with this PR's single refresh. Taking either side wholesale drops one of them.
  • ConceptVectors.index_name returns ConceptDocument._index._name. Until the first P1 is fixed, a rebuilt worker reads stored vectors from the renamed index. During a --use-alias rebuild 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()

Comment thread core/common/tasks.py Outdated
Comment thread core/common/tasks.py
Comment thread core/common/management/commands/es_split_index.py Outdated
Comment thread core/common/management/commands/es_split_index.py Outdated
Comment thread core/common/management/commands/es_split_index.py Outdated
Comment thread core/common/management/commands/es_split_index.py Outdated
snyaggarwal and others added 2 commits October 6, 2026 21:27
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>
@snyaggarwal

Copy link
Copy Markdown
Contributor Author

Summary of review followup

Review follow-up

Thanks for both reviews (Codex pass 1 and Claude). master is merged in. Status of each finding:

Finding Source Verdict Change
--use-alias renames the shared registry Index objects and never renames them back, so a later rebuild in the same worker can delete the index behind the alias Codex 2, Claude P1 ✅ Fixed rebuild_indexes saves every registry index name and puts it back in a finally, failures included. Test: test_rebuild_indexes_restores_the_index_names
rollback() can run after the swap went through and delete the only copy Codex 1, 5, Claude P1 ✅ Fixed The swap is sent with no client retry. On any error the command first checks whether the alias already points at the target. If so, the swap went through and nothing is rolled back. If ES can't answer, the target is kept. The write block is cleared even if deleting the target fails, and the error says when a rollback was incomplete. Ordinary calls get a 120 s timeout. Tests: test_does_not_roll_back_a_swap_that_went_through, test_keeps_the_target_when_the_swap_state_is_unknown, test_clears_the_block_even_when_deleting_the_target_fails
The write block doesn't drain in-flight writes; the first block request sits outside try; the prompt kept writes blocked while waiting for an answer Codex 4, 5, Claude P2 ✅ Fixed Uses indices.add_block(block='write'), which waits for in-flight writes, now inside the rollback guard. The confirmation comes before anything is blocked. The block start time is printed. Tests: test_clears_a_block_that_applied_but_timed_out, test_asks_before_blocking_writes
Writes during the block, or during a --use-alias rebuild, aren't replayed Codex 3, 4, Claude P2 📝 Documented The runbook pauses production-celery-indexing for the window, so most saves queue instead of failing. After the swap, re-index rows updated since the printed block time: POST /indexes/resources/<resource>/ with filter={"updated_at__gte": ...}. The command prints that hint. No replay mechanism is built in
The disk preflight allows merges past the ES disk watermarks Codex 7, Claude P2 ✅ Fixed Checks the projected peak, (disk.used + index size) / disk.total, against --max-disk-percent (default 85). A local split of a 21 GB index peaked at about 2× on disk before the merges released the hard links
search_index --populate/--rebuild still sends refresh=true with every bulk chunk (AC 3) Codex 9, Claude P2 ✅ Fixed refresh=False is passed to both. The 1 s refresh interval covers visibility
README documents a plain --rebuild, which deletes the indices behind the aliases Codex 8, Claude P2 ✅ Fixed README now uses --rebuild --use-alias and explains why
After #926, ConceptVectors reads stored vectors through the renamed index, so a rebuild re-encodes everything Codex, Claude (post-merge) ✅ Fixed ConceptVectors.index_name reads the alias (ConceptDocument.Index.name), which a rebuild doesn't rename
The runbook ends after the split, still pinned to one node with 0 replicas; it adds replicas before the split Codex 6, Claude P2 ✅ Fixed (docs) The runbook in ocl-online-docs infrastructure/es-shard-relayout.md now runs: expunge (wait for merges and docs.deleted ≈ 0) → unpin → replicas → green with copies on all 3 nodes. Replicas are no longer added before the split. The PR description is updated to match
A failed final refresh is only logged Codex 10 ⏭️ Not fixing These indices keep the default 1 s refresh interval, so a failed final refresh only delays visibility by about a second. Failing a long reindex over it would just trigger a costly rerun (same view as the Claude review)
tools/backfill_expansion_url_per_expansion.py refreshes on every bulk Codex 9 ⏭️ Not fixing It's a one-off script that counts in ES right after each bulk, so it needs the refresh

Verified:

  • pylint is 10.00 on the changed files.
  • core.common plus the vectorization tests pass: 257 tests.
  • End to end on a local single-node ES:
    • concepts, 3.4M docs, split 1 → 6 and mappings split 1 → 2, both behind aliases, with doc counts unchanged.
    • After the expunge merge, docs.deleted is 0 and the 21.4 GB index is down to 20 GB.
    • Refresh is down to 1 per job per shard (it was 1 per bulk).

@snyaggarwal

Copy link
Copy Markdown
Contributor Author

OCL Online docs PR -- https://github.com/OpenConceptLab/ocl-online-docs/pull/6

@paynejd paynejd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@paynejd

paynejd commented Oct 6, 2026

Copy link
Copy Markdown
Member

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.

  • Before the prod run: the runbook PR, ocl-online-docs#6, needs three fixes from its Codex pass (details there): disk headroom before the split, source visibility changes made during the write window, and how the shard env vars get deployed. This PR's description also still adds replicas before the split; point it at the runbook instead.
  • Not blocking: the terminal refresh before the alias swap. refresh=False was my pass-1 suggestion. With the default 1 s refresh interval, an idle shard refreshes on its first search, so results stay correct.

@snyaggarwal
snyaggarwal merged commit 0f7c4c0 into master Oct 7, 2026
3 checks passed
@snyaggarwal
snyaggarwal deleted the ocl_online#244 branch October 7, 2026 02:16
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.

2 participants