Skip to content

[Improvement] Index snapshot export/import commands - #513

Merged
mcop1 merged 74 commits into
2.5from
feature/index-snapshot-export-import
Sep 24, 2026
Merged

mcop1 merged 74 commits into
2.5from
feature/index-snapshot-export-import

Conversation

@mcop1

@mcop1 mcop1 commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

Changes in this pull request

Resolves #

Adds two console commands that let an installation's search indices be exported to a portable bundle and imported into another installation in minutes, without a full reindex:

  • generic-data-index:snapshot:export streams every GDI index (data-object folder, one per class, asset, document) into one gzipped JSON-lines file per index plus a manifest.json, written to a configurable Flysystem storage. Read-only against the search engine. Options: --name, --dry-run. The manifest records the index-queue depth before and after the export as a quality signal.
  • generic-data-index:snapshot:import [name] recreates the indices with the local mappings and replays the documents through the bulk API. It never enqueues elements. Before touching anything it compares the class mapping checksums in the manifest with the local settings store and class definitions; a mismatch is refused unless --force, which skips only the affected classes. Options: --from-path, --force, --dry-run.
  • New config node pimcore_generic_data_index.snapshot (storage, page_size, page_bytes, bulk_size, bulk_bytes) and a prepended, private, overridable default Flysystem storage pimcore.generic_data_index_snapshot.storage under var/generic-data-index/snapshots. Snapshots are exported on demand and kept until an operator deletes them; there is no automatic rotation.
  • Documentation page doc/02_Configuration/06_Index_Snapshots.md and upgrade notes.
  • Memory-safe paging (added after a large-dataset test): page_size/bulk_size are ceilings. The export probes each index with a single document and then sizes every page from the larger of the cumulative average and the most recent page's average of raw JSON written, so a page stays within page_bytes (16 MiB default) and a run of larger documents shrinks the next page immediately (the budget is a target, not a hard ceiling, since the engine cannot be asked for "at most N bytes"); the import flushes a bulk request as soon as either bulk_size documents or bulk_bytes are pending. A fixed 1000-document page had exhausted a 1 GB memory_limit on the first page of a class with large documents. Both commands log every page/flush at debug level; the docs recommend --no-debug, because GDI's debug-mode search history (see [Bug] Stop unbounded memory growth from executed search debug history (backport #418, #486) #516 for the 2.5 backport of Avoid accumulating executed search debug traces outside debug mode #418/Cap executed search debug history in debug mode #486) otherwise grows with every page.

Why: a full reindex of ~1.4M elements takes 3–4 h on production and a working day on a developer notebook. Since mappings are regenerated locally, a bundle is portable between OpenSearch and Elasticsearch and independent of the engine version. Developers import the bundle together with the matching database dump. Native engine snapshot repositories are not an option on Pimcore PaaS.

Backward compatibility: purely additive. Existing files touched: composer.json (declares league/flysystem, adds league/flysystem-memory to require-dev), Configuration.php (new node), the DI extension (prepend + wiring), two doc files.

Additional info

Verified locally in the bundle's Docker harness on this branch:

  • Combined Codeception run (codecept run, Functional + Unit): OK on every push (baseline on clean 2.5: 501 tests); the Snapshot functional folder grew to 34+ tests through the review rounds, covering data-object, asset and document indices.
  • Snapshot functional suite also run against the Elasticsearch service of the harness: OK, 21 tests (before the final refinement commits, which change no engine calls).
  • php-cs-fixer and PHPStan clean for the bundle.
  • Byte-budget paging: PageSizerTest (unit), a 2-document-ceiling/1-byte-budget export test asserting the exact page sequence, a 1-byte bulk budget round trip asserting one flush per document; Snapshot functional suite OK (33 tests) on this head.
  • Round 14 follow-ups: bulk replay extracted into DocumentReplayer (importer back under the 20-dependency Sonar limit), unverified classes render an empty manifest checksum instead of a sentinel 0, and the end-to-end round trip now creates/deletes/restores a real image asset.

Verified end to end on a 2.0M-document dataset (2026-09-22): snapshot exported on a colleague's 12.3-line server (28 indices, 2,009,355 documents, 831 MB, 185 s), imported into a fresh 2025.4 stack on a notebook (empty OpenSearch, database restored from the matching dump): 28/28 indices complete, 66 min, PHP peak 593 MiB under a 1 GB limit, all 25 class checksums stamped, queue untouched. A re-export of the imported cluster produced 28 files byte-identical (same SHA-256 and size) to the original snapshot.

Replay performance (2026-09-22): while an index is replayed its automatic refresh is disabled and the previous refresh_interval is restored afterwards (also after a failure); the translog keeps request durability, so every acknowledged bulk request is durable. Snapshot lines go into the bulk body unchanged (no decode/encode round trip; only the id is read). Measured on the 2.0M-document import: 3792 s vs 3992 s before; the remaining time was the single shard indexing sequential bulk requests. Parallel replay now ships in this PR: the importer cuts the snapshot file into bulk bodies on disk and sends them through import_workers (default 4) long-running worker processes (hidden generic-data-index:snapshot:replay-worker command, fed via stdin), so several bulk requests are in flight at once; rejected requests (HTTP 429) are retried with backoff. Measured on the same 2.0M documents: 4 concurrent writers 3.0× the single-writer throughput, 8 writers 3.4×, 12 writers rejected by the node. There is a single sending path: import_workers: 1 keeps one request in flight. Full import re-measured with import_workers: 4: 1193 s (vs 3792 s with one writer, 3.2×), 28/28 indices complete, settings restored, no chunk files left behind; the memory of the importing process plus its four workers peaked at 963 MiB in total. A re-export of the cluster after that parallel import is again byte-identical to the original snapshot (28/28 files, same SHA-256 and size), i.e. concurrency neither dropped nor duplicated a document. Simplified head (single worker-pool path, refresh-only replay mode, 2026-09-23): a fresh export of the same 2.0M documents imported in 1253 s on an idle notebook (1356 s with other load), ~10% slower than with an asynchronous translog; accepted in exchange for dropping the flush barrier and its failure handling.

Design notes

  • Manifest is written last; a directory without a manifest is treated as an aborted export and is invisible to listing and import.
  • Import downloads and verifies (name, sha256) the file of every index it is going to import before any index is touched; files of entries skipped under --force (incompatible/unverified/missing locally) are not downloaded, since they are never imported. The apply phase is not atomic across indices: indices before a failure are complete, the failing one is recreated and partial, later ones untouched; re-running repairs. The class checksum is only stamped after a complete replay so GDI's own per-class reindex still self-heals a failed import. Export and import share one lock.
  • The compatibility check always computes the local class checksum first; the settings store only decides between compatible and stale. A class index without a checksum entry is unverified and gated like a mismatch.
  • Snapshot names are validated against a strict pattern; manifest entries must reference their own <short_name>.ndjson.gz, carry a consistent element-type/class-id identity (IndexIdentity value object), and be unique. The manifest is treated as data (indices are mapped back by class id, never by name).
  • The export pages live aliases without a point-in-time view (documented non-goal); run it in a quiet window after the database dump.

Observed while working on this, not changed here (separate tickets): SearchIndexConfigService::getShortIndexName() returns the prefix instead of the remainder; SearchResultDenormalizer crashes when search() is called with trackTotalHits: false; .github/ci/files/config/pimcore/config.yaml lags the bundle config (isReferenced missing); BulkOperationService keeps its buffer when bulk() throws.

🤖 Generated with Claude Code

mcop1 and others added 20 commits September 15, 2026 11:17
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…dirs

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Applies the final review fix wave to the index snapshot feature:

- Stamp the class mapping checksum only after replay's bulk commit and
  count verification succeed, not right after provisioning, so a failed
  replay never marks an empty/partial index as current (SnapshotImporter).
- Add ClassCompatibilityStatus::UNVERIFIED and close the gate for class
  indices the manifest has no checksum for (CompatibilityChecker,
  CompatibilityReport, SnapshotImporter, SnapshotImportCommand), with a
  matching compatibility-table row in the docs.
- Warn on --only that a subset import can leave indices inconsistent.
- Move the exporter's success log outside the write-manifest try block.
- Wrap Flysystem failures around the download/verify/replay sequence as
  SnapshotImportException, and reject unsafe manifest file names.
- Minor cleanups: assert bulk_size in the configuration test, trailing
  newline in doc/02_Configuration/README.md, temp-file cleanup in
  SnapshotStorageTest, and parameterized SQL in SnapshotExporterTest.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 15, 2026 13:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Unresolved consistency, compatibility, locking, timeout, performance, and data-integrity defects can produce unsafe snapshots or imports.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds portable Flysystem-backed search-index snapshots, enabling export to compressed bundles and import using locally generated mappings.

Changes:

  • Adds snapshot export/import commands and supporting services.
  • Adds configuration, dependencies, documentation, and upgrade notes.
  • Adds unit and functional coverage for snapshot workflows.
File summaries
File Description
tests/Unit/Service/SearchIndex/Snapshot/SnapshotStorageTest.php Tests storage and rotation.
tests/Unit/Service/SearchIndex/Snapshot/QueueGateTest.php Tests queue gating.
tests/Unit/Service/SearchIndex/Snapshot/DocumentFileTest.php Tests compressed document files.
tests/Unit/Model/Snapshot/ManifestTest.php Tests manifest serialization.
tests/Unit/Model/Snapshot/CompatibilityReportTest.php Tests compatibility reporting.
tests/Unit/DependencyInjection/ConfigurationTest.php Tests snapshot configuration.
tests/Unit/Command/Snapshot/SnapshotImportCommandTest.php Tests import command behavior.
tests/Unit/Command/Snapshot/SnapshotExportCommandTest.php Tests export command behavior.
tests/Functional/Snapshot/SnapshotRoundTripTest.php Tests end-to-end restoration.
tests/Functional/Snapshot/SnapshotIndexResolverTest.php Tests local index resolution.
tests/Functional/Snapshot/SnapshotExporterTest.php Tests exporter integration.
tests/Functional/Snapshot/CompatibilityCheckerTest.php Tests mapping compatibility.
src/Service/SearchIndex/Snapshot/SnapshotStorageInterface.php Defines snapshot storage API.
src/Service/SearchIndex/Snapshot/SnapshotStorage.php Implements Flysystem storage.
src/Service/SearchIndex/Snapshot/SnapshotIndexResolverInterface.php Defines index resolution API.
src/Service/SearchIndex/Snapshot/SnapshotIndexResolver.php Resolves local index targets.
src/Service/SearchIndex/Snapshot/SnapshotImporterInterface.php Defines importer contract.
src/Service/SearchIndex/Snapshot/SnapshotImporter.php Provisions and replays indices.
src/Service/SearchIndex/Snapshot/SnapshotExporterInterface.php Defines exporter contract.
src/Service/SearchIndex/Snapshot/SnapshotExporter.php Streams indices into snapshots.
src/Service/SearchIndex/Snapshot/QueueGate.php Gates export on queue depth.
src/Service/SearchIndex/Snapshot/DocumentFileWriter.php Writes gzip JSON-lines files.
src/Service/SearchIndex/Snapshot/DocumentFileReader.php Verifies and reads snapshot files.
src/Service/SearchIndex/Snapshot/CompatibilityCheckerInterface.php Defines compatibility checks.
src/Service/SearchIndex/Snapshot/CompatibilityChecker.php Compares mapping checksums.
src/Model/Snapshot/WrittenFile.php Models exported file metadata.
src/Model/Snapshot/ManifestIndex.php Models manifest index entries.
src/Model/Snapshot/Manifest.php Models snapshot manifests.
src/Model/Snapshot/IndexTarget.php Models local index targets.
src/Model/Snapshot/ImportResult.php Models import results.
src/Model/Snapshot/ImportOptions.php Models import options.
src/Model/Snapshot/ImportedIndex.php Models imported index counts.
src/Model/Snapshot/ExportResult.php Models export results.
src/Model/Snapshot/ExportOptions.php Models export options.
src/Model/Snapshot/CompatibilityReport.php Aggregates compatibility statuses.
src/Model/Snapshot/ClassCompatibility.php Models class compatibility.
src/Exception/Snapshot/SnapshotIncompatibleException.php Adds incompatibility exception.
src/Exception/Snapshot/SnapshotImportException.php Adds import exception.
src/Exception/Snapshot/SnapshotExportException.php Adds export exception.
src/Exception/Snapshot/InvalidSnapshotException.php Adds validation exception.
src/Enum/Snapshot/ClassCompatibilityStatus.php Defines compatibility states.
src/DependencyInjection/PimcoreGenericDataIndexExtension.php Wires snapshot services and storage.
src/DependencyInjection/Configuration.php Adds snapshot configuration nodes.
src/Command/Snapshot/SnapshotImportCommand.php Adds snapshot import command.
src/Command/Snapshot/SnapshotExportCommand.php Adds snapshot export command.
doc/02_Configuration/README.md Links snapshot documentation.
doc/02_Configuration/06_Index_Snapshots.md Documents snapshot operation.
doc/01_Installation/02_Upgrade.md Adds upgrade notes.
config/services/snapshot.yaml Registers snapshot services.
composer.json Adds Flysystem dependencies.
Review details
  • Files reviewed: 50/50 changed files
  • Comments generated: 7
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/Command/Snapshot/SnapshotImportCommand.php Outdated
Comment thread src/Model/Snapshot/ManifestIndex.php
Comment thread src/Service/SearchIndex/Snapshot/CompatibilityChecker.php Outdated
Comment thread src/Service/SearchIndex/Snapshot/DocumentFileWriter.php Outdated
Comment thread src/Service/SearchIndex/Snapshot/SnapshotExporter.php
Comment thread src/Service/SearchIndex/Snapshot/QueueGate.php Outdated
Comment thread src/Service/SearchIndex/Snapshot/SnapshotExporter.php Outdated
Wraps every line over 120 characters and reformats multi-argument
calls one-argument-per-line across the snapshot export/import feature
(src/Command/Snapshot, src/Service/SearchIndex/Snapshot,
src/Model/Snapshot, and the two DependencyInjection files).

Reduces cognitive complexity below the gate threshold via pure
extractions, no behaviour change:
- CompatibilityChecker::check() -> checkClass(), collectUncheckedClassIndices(),
  collectClassesMissingInManifest()
- SnapshotImporter::import() -> collectNotices(), plan()
- SnapshotImportCommand::execute() -> resolveStorage(), renderResult(),
  renderIncompatible()

Documents the deliberately ASCII-only file-name pattern in
SnapshotImporter::replay() (manifest file names are always generated
by the exporter from ASCII short names).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@mcop1 mcop1 self-assigned this Sep 15, 2026
mcop1 and others added 2 commits September 15, 2026 15:42
- Share a named lock between export and import so they can never run concurrently
  (SnapshotExportCommand, SnapshotImportCommand).
- Require document_count/bytes (ManifestIndex) and queue_entries_before/after/duration_seconds
  (Manifest) to be non-negative integers, and class_mapping_checksums values to be integers,
  instead of silently coercing with (int).
- CompatibilityChecker now always computes the checksum from the current local class definition
  and lets that decide COMPATIBLE/STALE_STORE/INCOMPATIBLE, since the stored checksum can lag an
  asynchronously dispatched class-mapping update.
- DocumentFileWriter::finish() checks gzclose()'s return value and fails loudly (unlinking the
  partial file) instead of silently accepting a corrupt gzip stream.
- QueueGate::await() sleeps only for the remaining wait budget instead of always sleeping a full
  poll interval, so --wait is honored precisely.
- SnapshotExporter passes an integer track_total_hits bound instead of `true`, avoiding a full
  count on every export page.
- Document the export's lack of a point-in-time snapshot and how to read the manifest's queue
  counts as a cleanliness indicator.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
SonarCloud flagged php:S1200 on SnapshotImporter (23 class dependencies, max 20). Extract the
per-element-type index handlers and the settings-store checksum stamping into a new
IndexProvisioner/IndexProvisionerInterface pair, owned independently of the import decision logic
(verify before provision, stamp only on complete replay), which stays in SnapshotImporter.

- IndexProvisioner::provision() recreates the local index for a target (existsAlias -> deleteIndex,
  updateMapping with forceCreateIndex).
- IndexProvisioner::computeClassMappingChecksum() returns the checksum for a class index, null
  otherwise.
- IndexProvisioner::stampClassMapping() stores the checksum once a replay has fully succeeded.
- SnapshotImporter no longer depends on the three IndexHandlers, IndexHandlerInterface, ElementType,
  or SettingsStoreServiceInterface directly; it only depends on IndexProvisionerInterface, a net
  reduction of 5 class dependencies.
- Registered IndexProvisionerInterface -> IndexProvisioner in config/services/snapshot.yaml
  (private, matching the bundle's existing service defaults).
- Updated the Import compatibility table in doc/02_Configuration/06_Index_Snapshots.md for the
  CompatibilityChecker rule from the previous round: the local class definition is always the
  reference, the settings store only decides compatible vs. stale.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Bulk refresh behavior, inaccurate missing-class reporting, and masked export failures need correction.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 52/52 changed files
  • Comments generated: 3
  • Review effort level: Balanced

Comment thread src/Service/SearchIndex/Snapshot/CompatibilityChecker.php
Comment thread src/Service/SearchIndex/Snapshot/SnapshotExporter.php
Comment thread src/Service/SearchIndex/Snapshot/SnapshotImporter.php Outdated
@mcop1
mcop1 requested a balanced review from Copilot September 22, 2026 18:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

A failed settings update can leave an index in bulk-loading mode, and the documented temporary-disk bound is inaccurate.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (7)
Previously missed (1)

In code that hasn't changed since last review

Low severity Document temporary disk-space bound assumptions and overhead

doc/​02_Configuration/​06_Index_Snapshots.md:71

This disk bound is not guaranteed: chunk sizing counts only snapshot document-line bytes, while the files also contain one bulk action line per document, and a single document larger than bulk_bytes is intentionally emitted as an oversized chunk. Operators therefore cannot use 2 × import_workers × bulk_bytes as a hard temporary-space limit. Qualify the estimate and mention both sources of overhead.

Comment thread src/Service/SearchIndex/Snapshot/ReplayIndexSettings.php Outdated
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The destructive, concurrent cross-engine import path and broad operational impact warrant final human validation despite the extensive safeguards and tests.

Review effort: Balanced
Findings: None

Resolved since last review (1)

…ribe what the matching database dump must contain

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The in-process replay path can leave bulk chunk files behind when reading a chunk fails.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Medium severity Clean up temporary chunk files when reading fails

src/​Service/​SearchIndex/​Snapshot/​InProcessBulkDispatcher.php:57

If reading the chunk fails, the exception is thrown before the finally block, and abort() is a no-op for this dispatcher. The generated temp file can therefore be left behind, unlike both the worker dispatcher and send-failure path. Put the read inside the same try/finally so every dispatch attempt removes its chunk.

mcop1 and others added 2 commits September 23, 2026 08:56
…bulk replay

DocumentFileReader::read()/readLines() and the DocumentLine value object
were only called from tests since the replay streams raw lines. The
malformed-line coverage moves to BulkChunkWriterTest, where invalid JSON
is actually rejected now.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…mode is refresh only

- The import always sends through WorkerPoolBulkDispatcher (import_workers
  >= 1); BulkDispatcherInterface and InProcessBulkDispatcher are gone,
  BulkDispatcherFactory only builds the pool. With one worker the only
  cost is booting one extra process.
- ReplayIndexSettings only disables refresh_interval for the replay and
  restores the previous value. The translog keeps request durability, so
  every acknowledged bulk request is durable: the flush barrier, its
  failed-shard check, the reset after a failed flush and the
  IndexSettingsBackup value object are no longer needed. With several
  writers in flight the engine batches translog fsyncs anyway.
- DocumentReplayerTest drives the real pool with a fake bin/console.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The importer ignores failed shards reported by the final refresh and may validate or stamp an incompletely visible index.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)

Comment thread src/Service/SearchIndex/Snapshot/SnapshotImporter.php Outdated
Copilot AI previously approved these changes Sep 23, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approved

The implementation is well-covered and functionally coherent; only a minor configuration-help wording correction remains.

Review effort: Balanced
Findings: 1 Low severity

Open (1)
Resolved since last review (1)

Comment thread src/DependencyInjection/Configuration.php Outdated
…config help

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Worker-pool failure tests contain scheduler-dependent assertions that can fail nondeterministically in CI.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Assertion includes chunks never dispatched

tests/​Unit/​Service/​SearchIndex/​Snapshot/​WorkerPoolBulkDispatcherTest.php:84

This assertion includes chunks the loop may never hand to the dispatcher. An immediate worker failure can surface from dispatch() before later iterations run, so those later files remain caller-owned and this test fails nondeterministically. Track and assert only the chunks whose dispatch was attempted; the separate current-chunk test covers cleanup of the call that surfaces the failure.

This issue also appears on line 91 of the same file.

…duling

A worker failure can surface from any dispatch() call, so chunks the loop
never handed over stay with the caller. Both failure tests now assert
cleanup only for chunks that were actually handed to the pool, including
the chunk of the call that surfaced the failure. (Copilot, suppressed note)

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The documentation understates temporary-disk requirements by omitting retained preflight copies of the complete snapshot bundle.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Low severity Account for preflight snapshot copies in disk sizing

doc/​02_Configuration/​06_Index_Snapshots.md:72

This disk-sizing guidance omits the preflight copies retained by SnapshotImporter::preflight(): every planned compressed index file is downloaded into the system temp directory before apply starts, so peak temp usage includes the entire compressed bundle (and --from-path duplicates it) in addition to queued bulk chunks. Large snapshots can otherwise exhaust ephemeral disk before import begins; document that capacity requirement here.

…t models

The Pimcore coding guidelines make every property private by default
(must) and immutable, set via the constructor, without setters (must);
all other models of this bundle follow that with private properties and
getters. The 14 snapshot models and SnapshotIncompatibleException exposed
their state as public readonly properties instead. They are private now,
with getters (isDryRun()/isForce() for the booleans), and every read uses
the getter. No behaviour change.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The destructive multi-process import path and broad operational surface warrant final human review despite comprehensive coverage.

Review effort: Balanced
Findings: 1 Low severity

Open (1)

Comment thread tests/Functional/Snapshot/SnapshotRoundTripTest.php Outdated
…ability

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The destructive, concurrent cross-engine import path warrants final human review despite extensive testing.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Low severity Preserve pending chunk count before aborting

src/​Service/​SearchIndex/​Snapshot/​WorkerPoolBulkDispatcher.php:222

abort() clears $inFlight before this value is read, so a worker that dies with pending work is always reported as having 0 chunk(s) pending. Capture the count before aborting and use that value in the exception so the diagnostic reflects the actual lost work.

v2.5.12 was released without this feature; the milestone is now 2.5.13.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… the real number of lost chunks (#520)

pump() read a worker's output before checking whether it still ran. A
worker that wrote its last answer and exited in between was taken for
one that died with chunks pending, failing a healthy import. The running
state is now checked first; everything written before the exit is read
and handled afterwards.

The "worker died" message read the pending count after abort() had
cleared it and always said "0 chunk(s)"; the count is captured first.

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The fixed-duration lock can expire during a long preflight or index replay, permitting conflicting snapshot operations.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)

Comment thread src/Service/SearchIndex/Snapshot/SnapshotLock.php

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Manifest validation can accept incomplete snapshots that silently leave stale singleton indices untouched.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Reject manifests missing required singleton indices

src/​Model/​Snapshot/​Manifest.php:247

The parser accepts a manifest with an empty indices list or with the asset, document, or data-object-folder entry removed. Because import only provisions listed entries, that snapshot can report success while the corresponding stale local index remains untouched—the same outcome the exporter’s empty-index entries are intended to prevent. Reject manifests that omit any universal singleton index.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The destructive cross-engine import and multi-process replay path warrants final human operational review despite comprehensive safeguards and tests.

Review effort: Balanced
Findings: None

mcop1 and others added 2 commits September 24, 2026 11:02
An additional option next to the bundle import: import the bundle once
into a separate installation, take a native OpenSearch/Elasticsearch
snapshot there, and restore that natively wherever it is needed, together
with the matching database dump. Covers what the importing installation
needs, the repository setup, the snapshot/restore API calls, and the
engine-version and index-prefix constraints.

Verified on a notebook with a 2.0M-document import: snapshot 252 s,
restore 252 s, all 28 indices with identical document counts and all 82
alias entries identical afterwards.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The destructive, non-atomic import and multi-process replay path warrant final human review despite strong test coverage.

Review effort: Balanced
Findings: None

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants