issue #617: add indexing-admin delete-segment command for corrupted segments#623
Merged
eolivelli merged 3 commits intoMay 21, 2026
Merged
Conversation
…egments Adds an operator remediation tool that removes a single segment from the IS in-memory metadata after a Phase B upload failure has left it registered without a complete graph file in remote storage. Without this, every subsequent compaction cycle re-selects the broken segment and fails with S3 NoSuchKey, locking the index into a compaction livelock that only a full cluster teardown can resolve. The new path: - `DeleteSegment` gRPC RPC + proto messages, surfaced as the `indexing-admin delete-segment` CLI sub-command with `--segment`, `--purge-storage`, `--force`, and `--yes` flags (plus an interactive stdin confirmation for non-scripted use). - Engine-level `IndexingServiceEngine.deleteSegment` that locates the store, verifies presence, refuses the delete when the graph file is still reachable (unless `force=true`), drops the segment from the in-memory list, optionally purges multipart files, and force-runs a checkpoint so the new (smaller) segment list is serialised to IndexStatus and republished to ZK — shadows observe the change on their next reload. - `PersistentVectorStore.dropSegmentByStorageKey` reuses the same writeLock + deferred-close protocol as `dropSegmentByUuid` (issue #535), so the operator path is safe against concurrent compaction. - Best-effort `DataStorageManager.multipartIndexFileExists` default that probes the first 4 bytes via the multipart reader supplier — used as the safety gate without requiring backend-specific HEAD APIs. Tests: - `IndexingAdminCliDeleteSegmentTest` — 8 fake-gRPC tests covering flag forwarding (purge/force/--yes), text vs JSON output, exit codes (0 for removed=true, 1 for removed=false), server-refusal mapping, stdin confirmation accept/abort, and missing-required-flag usage handling. - `ShadowDeleteSegmentE2ETest` — 3 end-to-end tests with a real ZKTestEnv + MemoryDataStorageManager + PersistentVectorStore: the refusal gate when the graph file is reachable, the force+purge path with shadow-replica notification (shadow reloadCount advances after the post-delete republish), the no-purge path that keeps multipart files for forensics, and the unknown-segment rejection diagnostic. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Six follow-ups from the first pr-reviewer pass:
1. Add an owner-instance / shadow-target gate in
IndexingServiceImpl.deleteSegment: short-circuit with
FAILED_PRECONDITION ("not_primary: ... shadowOf=...") when the engine
is configured as a shadow, so an accidental operator-targeted delete
on a shadow cannot diverge it from its primary. Adds a fake-gRPC CLI
test that asserts the rejection prefix and non-zero exit code.
2. Add RemoteFileDataStorageManagerProbeAndPurgeTest covering the
probe-and-purge contract: absent → false, fully-present → true,
deleteMultipartIndexFile is idempotent + invalidates the
SegmentBlockCache (verified via the public containsBlock accessor),
and a partial block-0 (≥ 4 bytes, truncated rest) still returns true
— documenting the contract gap that --force covers. Fixes a latent
bug in the default multipartIndexFileExists impl
(DataStorageManager) where fileSize=1L would cause an infinite loop
in RemoteRandomAccessReader.readFully; the default sentinel is now
1 GiB and RemoteFileDataStorageManager overrides the probe to use a
wire-level readFileRange instead.
3. Add PersistentVectorStoreAdminDeleteSegmentTest with four cases:
happy-path removal (segment gone + dirty marked),
idempotent NOT_FOUND on second call, deferred-close drain
(pendingSegmentCloses queue empties via opportunistic drain), and
compaction-cycle race safety (the storage-key drop path inherits
issue #535's deferred-close protocol → no CompactionException).
Surfaces the queue size via a new test-only accessor on
PersistentVectorStore.
4. Add lateBootShadowObservesPostDeleteState to ShadowDeleteSegmentE2ETest:
primary deletes a segment BEFORE the shadow is ever started, then
the shadow boots and is verified to load the smaller (post-delete)
segment count on its very first reload (shadowReloadCount == 1).
Surfaces the shadow's segment count via a new IndexingServiceEngine
test accessor that works for both PersistentVectorStore (primary)
and ReadOnlyVectorStore (shadow).
5. Resolve the vectors_lost=-1 doc drift: the race path (segment
disappeared between snapshot and drop) now returns -1L so operators
can distinguish "removed 0 vectors" from "did not remove anything
and cannot tell what would have been lost". Proto comment updated
accordingly.
6. Tighten the rejection message in IndexingServiceEngine.deleteSegment
so when store instanceof ReadOnlyVectorStore it says "this instance
is a shadow replica — target the primary indexing service" rather
than the generic "non-persistent" message. Belt-and-braces in case
the IS-level gate at #1 is ever bypassed.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Iteration 3 of 3: - #1 (race-path test): add IndexingServiceEngineDeleteSegmentEdgeCasesTest with `engineReportsRaceSentinelWhenSegmentDroppedConcurrently` that drives the engine into the removed=false / vectors_lost=-1 branch via a new package-private hook `setPreDropRaceHookForTests` fired inside `deleteSegment` between the engine's snapshot+probe and its own drop. - #2 (engine-only ReadOnlyVectorStore rejection): same test class adds `engineRefusesDeleteOnReadOnlyVectorStoreWithDiagnosticMessage` that registers a real ReadOnlyVectorStore directly on the engine and asserts the rejection message contains both "shadow replica" and "primary". - #3 (CLI -1 rendering): IndexingAdminCli now renders negative vectors_lost as `vectors_lost=unknown (race)` in text mode and as the string `"unknown"` in JSON (key kept so the schema stays stable). Two new tests in IndexingAdminCliDeleteSegmentTest cover both forms. - #4 (real-server shadow gate): add ShadowDeleteSegmentRpcGateTest with a real IndexingServiceImpl + real gRPC server + ShadowStubEngine whose `deleteSegment` throws AssertionError — proving the production `not_primary:` gate at IndexingServiceImpl short-circuits before reaching the engine. Asserts FAILED_PRECONDITION + the documented prefix + "target the primary instead". - #5 (multipartIndexFileExists override): override added in PromotableRemoteFileDataStorageManager (delegates to activeDelegate) and ReadReplicaDataStorageManager (same readFileRange shortcut as RemoteFileDataStorageManager). - #6 (nit): replaced the remaining `herddb.indexing.vector.ReadOnlyVectorStore` FQN references in IndexingServiceEngine with the imported simple name. - #7 (nit): reformat the 1 GiB literal in DataStorageManager to `1L << 30`. Nit #8 (full hammer suite) was deliberately skipped per the agent's "Never run the full test suite" rule — none of the changes in this commit touch indexes/checkpoints/concurrency hot paths (the checkpoint-on-delete behaviour was already present in the prior commit). Pre-PR validation (`spotless:check apache-rat:check spotbugs:check install -DskipTests -Pci`) is green across all 22 reactor modules. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #617.
Changes
Adds an operator remediation tool that removes a single segment from
the IS in-memory metadata after a Phase B upload failure has left it
registered without a complete graph file in remote storage. Without
this, every subsequent compaction cycle re-selects the broken segment
and fails with
S3 NoSuchKey, locking the index into a compactionlivelock that only a full cluster teardown can resolve.
DeleteSegmentgRPC RPC (+ proto messages) surfaced as theindexing-admin delete-segmentCLI sub-command with--segment,--purge-storage,--force, and--yesflags. A stdinconfirmation prompt protects against typos in interactive use;
--yesis the script-friendly skip.IndexingServiceEngine.deleteSegmentlocates the store, verifiesthe segment is registered, refuses the delete when the segment's
graph file is reachable in remote storage (unless
force=true),drops it from the in-memory list, optionally purges the multipart
graph + map files, and force-runs a checkpoint so the new (smaller)
segment list is serialised to
IndexStatusand republished to ZK —shadow replicas observe the change on their next reload.
PersistentVectorStore.dropSegmentByStorageKeyreuses the writeLockdropSegmentByUuid(issue [IS] runCompactionCycle aborts entire cycle when one candidate segment has no on-disk graph — infinite failure loop #535), sothe operator path is safe against concurrent compaction.
DataStorageManager.multipartIndexFileExistsdefaultprobes the first 4 bytes via the multipart reader supplier — used
as the safety gate without requiring backend-specific HEAD APIs.
Tests
IndexingAdminCliDeleteSegmentTest— 8 fake-gRPC tests coveringflag forwarding (purge/force/--yes), text vs JSON output, exit codes
(0 for
removed=true, 1 forremoved=false), server-refusalmapping, stdin confirmation accept/abort, and missing-required-flag
usage handling.
ShadowDeleteSegmentE2ETest— 3 end-to-end tests with a realZKTestEnv+MemoryDataStorageManager+PersistentVectorStore:the refusal gate when the graph file is reachable, the
force=true, purge_storage=truepath including shadow-replicanotification (shadow
reloadCountmust advance after thepost-delete republish), the no-purge path that keeps multipart
files for forensics, and the unknown-segment rejection diagnostic.
OnDiskSegmentMemoryAccountingTest,PersistentVectorStoreSegmentsQueriedTest,SegmentUuidRestartDurabilityTest,IndexingServiceEngineSegmentRegistryWiringTest,BLinkConcurrentSearchInsertTest, plus the existingIndexingAdminCli*andShadowE2ETestsuites all still pass onthe changed branch.
🤖 Implemented by the
pr-workeragent.