Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Dec 15, 2025

Index rebuild operations were failing because the Fjall adapter didn't handle deleted partitions properly. When rebuild_index drops an index partition and immediately recreates it, the adapter's cache retained stale entries and failed on the reopening with PartitionDeleted errors.

Changes

  • Error mapping: Added PartitionDeleted and "deleted" patterns to to_nitrite_error(), mapping them to StoreNotInitialized

  • Cache cleanup in open_map(): Remove stale registry entries when encountering deleted partitions during open operations

  • Graceful deletion in remove_map(): Handle already-deleted partitions by checking error messages and cleaning up registry instead of propagating errors

  • Helper function: Extracted is_partition_deleted_error() to centralize partition deletion detection logic

// Before: PartitionDeleted error propagated
Err(err) => {
    log::error!("Failed to open partition: {}", err);
    Err(to_nitrite_error(err))
}

// After: Clean up stale cache entry
Err(err) => {
    let err_msg = err.to_string();
    if Self::is_partition_deleted_error(&err_msg) {
        self.map_registry.remove(name);
    }
    log::error!("Failed to open partition: {}", err);
    Err(to_nitrite_error(err))
}

The changes ensure the adapter's map_registry cache stays consistent with on-disk partition state during drop/recreate cycles.

Original prompt

Problem

The test_rebuild_index integration tests are failing with a PartitionDeleted error when attempting to rebuild indexes. The error occurs in the following tests:

  • repository::repository_modification_test::test_rebuild_index
  • collection::single_field_index_test::test_rebuild_index
  • collection::single_field_index_test::test_rebuild_index_on_running_index

Root Cause

When rebuild_index is called, it performs the following steps:

  1. Drops the existing index by calling indexer.drop_index() which deletes the Fjall partition
  2. Attempts to recreate the index by reopening the partition

However, the Fjall store adapter has issues with this flow:

  • The map_registry cache retains stale entries after partition deletion
  • The remove_map function doesn't handle cases where partitions are already deleted
  • The open_map function doesn't properly clean up stale cache entries when encountering deleted partitions
  • Error handling in to_nitrite_error doesn't recognize PartitionDeleted errors

This causes the rebuild operation to fail when trying to reopen a just-deleted partition.

Solution

Implement the following fixes in the Fjall adapter:

1. Fix remove_map function in nitrite-fjall-adapter/src/store.rs

Update the function to handle cases where partitions may already be deleted:

fn remove_map(&self, name: &str) -> NitriteResult<()> {
    // close the map if it is open to drop any partition handles holding by the map
    self.close_map(name)?;
    
    if let Some(ks) = self.keyspace.get() {
        let options = self.store_config.partition_config();
        match ks.open_partition(name, options) {
            Ok(partition) => {
                match ks.delete_partition(partition.clone()) {
                    Ok(_) => {
                        // Ensure the map is removed from registry after successful deletion
                        self.map_registry.remove(name);
                        Ok(())
                    }
                    Err(err) => {
                        log::error!("Failed to remove partition: {}", err);
                        Err(to_nitrite_error(err))
                    }
                }
            }
            Err(err) => {
                // If partition doesn't exist, it might already be deleted
                // This is acceptable - just ensure it's removed from registry
                let err_msg = err.to_string();
                if err_msg.contains("not found") || err_msg.contains("deleted") || err_msg.contains("PartitionDeleted") {
                    self.map_registry.remove(name);
                    Ok(())
                } else {
                    log::error!("Failed to open partition for removal: {}", err);
                    Err(to_nitrite_error(err))
                }
            }
        }
    } else {
        Err(NitriteError::new(
            "Keyspace is not initialized",
            ErrorKind::PluginError,
        ))
    }
}

2. Update to_nitrite_error in nitrite-fjall-adapter/src/wrapper.rs

Add specific handling for partition deleted errors:

pub(crate) fn to_nitrite_error(error: impl Error) -> NitriteError {
    let error_msg = error.to_string();
    let error_kind = if error_msg.contains("closed") {
        ErrorKind::StoreAlreadyClosed
    } else if error_msg.contains("not found") {
        ErrorKind::StoreNotInitialized
    } else if error_msg.contains("deleted") || error_msg.contains("PartitionDeleted") {
        // Partition was deleted - treat as not found/not initialized
        ErrorKind::StoreNotInitialized
    } else if error_msg.contains("corrupt") {
        ErrorKind::FileCorrupted
    } else if error_msg.contains("permission") {
        ErrorKind::PermissionDenied
    } else if error_msg.contains("full") {
        ErrorKind::DiskFull
    } else {
        ErrorKind::BackendError
    };
    NitriteError::new(&format!("Fjall Error: {}", error_msg), error_kind)
}

3. Improve open_map in nitrite-fjall-adapter/src/store.rs

Ensure stale cache entries are cleaned up when encountering deleted partitions:

fn open_map(&self, name: &str, fjall_store: FjallStore) -> NitriteResult<NitriteMap> {
    let mut closed = false;
    if let Some(map) = self.map_registry.get(name) {
        if map.is_closed()? {
            closed = true;
        } else {
            return Ok(NitriteMap::new(map.clone()));
        }
    }

    // remove the map from registry if it is closed
    if closed {
        self.map_registry.remove(name);
    }

    if let Some(ks) = self.keyspace.get() {
        match ks.open_partition(name, self.store_config.partition_config()) {
            Ok(partition) => {
                let fjall_map = FjallMap::new(
                    name.to_string(),
                    partition,
                    fjall_store,
                    self.store_config.clone(),
                );
                fjall_map.initialize()?;

                self.map_regi...

</details>



<!-- START COPILOT CODING AGENT SUFFIX -->

*This pull request was created as a result of the following prompt from Copilot chat.*
> ## Problem
> 
> The `test_rebuild_index` integration tests are failing with a `PartitionDeleted` error when attempting to rebuild indexes. The error occurs in the following tests:
> - `repository::repository_modification_test::test_rebuild_index`
> - `collection::single_field_index_test::test_rebuild_index`
> - `collection::single_field_index_test::test_rebuild_index_on_running_index`
> 
> ### Root Cause
> 
> When `rebuild_index` is called, it performs the following steps:
> 1. Drops the existing index by calling `indexer.drop_index()` which deletes the Fjall partition
> 2. Attempts to recreate the index by reopening the partition
> 
> However, the Fjall store adapter has issues with this flow:
> - The `map_registry` cache retains stale entries after partition deletion
> - The `remove_map` function doesn't handle cases where partitions are already deleted
> - The `open_map` function doesn't properly clean up stale cache entries when encountering deleted partitions
> - Error handling in `to_nitrite_error` doesn't recognize `PartitionDeleted` errors
> 
> This causes the rebuild operation to fail when trying to reopen a just-deleted partition.
> 
> ## Solution
> 
> Implement the following fixes in the Fjall adapter:
> 
> ### 1. Fix `remove_map` function in `nitrite-fjall-adapter/src/store.rs`
> 
> Update the function to handle cases where partitions may already be deleted:
> 
> ```rust
> fn remove_map(&self, name: &str) -> NitriteResult<()> {
>     // close the map if it is open to drop any partition handles holding by the map
>     self.close_map(name)?;
>     
>     if let Some(ks) = self.keyspace.get() {
>         let options = self.store_config.partition_config();
>         match ks.open_partition(name, options) {
>             Ok(partition) => {
>                 match ks.delete_partition(partition.clone()) {
>                     Ok(_) => {
>                         // Ensure the map is removed from registry after successful deletion
>                         self.map_registry.remove(name);
>                         Ok(())
>                     }
>                     Err(err) => {
>                         log::error!("Failed to remove partition: {}", err);
>                         Err(to_nitrite_error(err))
>                     }
>                 }
>             }
>             Err(err) => {
>                 // If partition doesn't exist, it might already be deleted
>                 // This is acceptable - just ensure it's removed from registry
>                 let err_msg = err.to_string();
>                 if err_msg.contains("not found") || err_msg.contains("deleted") || err_msg.contains("PartitionDeleted") {
>                     self.map_registry.remove(name);
>                     Ok(())
>                 } else {
>                     log::error!("Failed to open partition for removal: {}", err);
>                     Err(to_nitrite_error(err))
>                 }
>             }
>         }
>     } else {
>         Err(NitriteError::new(
>             "Keyspace is not initialized",
>             ErrorKind::PluginError,
>         ))
>     }
> }
> ```
> 
> ### 2. Update `to_nitrite_error` in `nitrite-fjall-adapter/src/wrapper.rs`
> 
> Add specific handling for partition deleted errors:
> 
> ```rust
> pub(crate) fn to_nitrite_error(error: impl Error) -> NitriteError {
>     let error_msg = error.to_string();
>     let error_kind = if error_msg.contains("closed") {
>         ErrorKind::StoreAlreadyClosed
>     } else if error_msg.contains("not found") {
>         ErrorKind::StoreNotInitialized
>     } else if error_msg.contains("deleted") || error_msg.contains("PartitionDeleted") {
>         // Partition was deleted - treat as not found/not initialized
>         ErrorKind::StoreNotInitialized
>     } else if error_msg.contains("corrupt") {
>         ErrorKind::FileCorrupted
>     } else if error_msg.contains("permission") {
>         ErrorKind::PermissionDenied
>     } else if error_msg.contains("full") {
>         ErrorKind::DiskFull
>     } else {
>         ErrorKind::BackendError
>     };
>     NitriteError::new(&format!("Fjall Error: {}", error_msg), error_kind)
> }
> ```
> 
> ### 3. Improve `open_map` in `nitrite-fjall-adapter/src/store.rs`
> 
> Ensure stale cache entries are cleaned up when encountering deleted partitions:
> 
> ```rust
> fn open_map(&self, name: &str, fjall_store: FjallStore) -> NitriteResult<NitriteMap> {
>     let mut closed = false;
>     if let Some(map) = self.map_registry.get(name) {
>         if map.is_closed()? {
>             closed = true;
>         } else {
>             return Ok(NitriteMap::new(map.clone()));
>         }
>     }
> 
>     // remove the map from registry if it is closed
>     if closed {
>         self.map_registry.remove(name);
>     }
> 
>     if let Some(ks) = self.keyspace.get() {
>         match ks.open_partition(name, self.store_config.partition_config()) {
>             Ok(partition) => {
>                 let fjall_map = FjallMap::new(
>                     name.to_string(),
>                     partition,
>                     fjall_store,
>                     self.store_config.clone(),
>                 );
>                 fjall_map.initialize()?;
> 
>                 self.map_registry
>                     .insert(name.to_string(), fjall_map.clone());
>                 Ok(NitriteMap::new(fjall_map))
>             }
>             Err(err) => {
>                 // If partition was deleted, remove from cache and propagate error
>                 let err_msg = err.to_string();
>                 if err_msg.contains("deleted") || err_msg.contains("PartitionDeleted") {
>                     self.map_registry.remove(name);
>                 }
>                 log::error!("Failed to open partition: {}", err);
>                 Err(to_nitrite_error(err))
>             }
>         }
>     } else {
>         Err(NitriteError::new(
>             "Keyspace is not initialized",
>             ErrorKind::PluginError,
>         ))
>     }
> }
> ```
> 
> ## Expected Outcome
> 
> After these changes:
> 1. Index rebuild operations should complete successfully
> 2. Stale partition cache entries are properly cleaned up
> 3. The adapter gracefully handles deleted partitions during removal
> 4. All three failing tests should pass:
>    - `repository::repository_modification_test::test_rebuild_index`
>    - `collection::single_field_index_test::test_rebuild_index`
>    - `collection::single_field_index_test::test_rebuild_index_on_running_index`
> 
> ## Testing
> 
> The fix should be validated by running the failing integration tests:
> ```bash
> cargo test --tests --manifest-path ./Cargo.toml test_rebuild_index
> ```
> 
> All tests should pass after the implementation.
> 

<!-- START COPILOT CODING AGENT TIPS -->
---

💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey).

@coderabbitai
Copy link

coderabbitai bot commented Dec 15, 2025

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.


Note

Free review on us!

CodeRabbit is offering free reviews until Wed Dec 17 2025 to showcase some of the refinements we've made.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copilot AI changed the title [WIP] Fix PartitionDeleted error during index rebuild tests Fix PartitionDeleted error in index rebuild operations Dec 15, 2025
Copilot AI requested a review from anidotnet December 15, 2025 07:43
@anidotnet anidotnet marked this pull request as ready for review December 15, 2025 08:05
Copilot AI review requested due to automatic review settings December 15, 2025 08:05
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a critical bug in the Fjall adapter where index rebuild operations fail due to improper handling of deleted partitions. The adapter's in-memory cache (map_registry) was retaining stale entries after partition deletion, causing PartitionDeleted errors when attempting to recreate indexes.

Key changes:

  • Added error detection and mapping for deleted partitions in the error handling layer
  • Implemented cache cleanup logic when encountering deleted partitions during open and remove operations
  • Introduced a helper function to centralize partition deletion error detection

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
nitrite-fjall-adapter/src/wrapper.rs Added mapping of "deleted" and "PartitionDeleted" error patterns to StoreNotInitialized error kind
nitrite-fjall-adapter/src/store.rs Added helper function is_partition_deleted_error() and integrated cache cleanup logic in open_map() and remove_map() to handle deleted partitions gracefully

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@anidotnet anidotnet merged commit b08ab3f into main Dec 16, 2025
5 checks passed
@anidotnet anidotnet deleted the copilot/fix-rebuild-index-tests branch December 16, 2025 18:13
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