Fix erroneous removal of auth table during tree cleanup (#277)

* Fix erroneous removal of auth table during tree cleanup

When it comes to the handling of system tables, the current storage engine has
a little "funk." It is because the limited flexibility of the format prevents
us from directly storing system tables which have a different structure than
ordinary user created tables; this is why when the cleanup is run, previously,
the dummy keyspace created for the PRELOAD was scanned and since it has nothing
any "alien" file in the `ks/system` directory was purged. This commit fixes
that and ensures that we do not clean up the system directory.

This is also another reason why a new storage engine coupled with a new memory
engine are being developed. These shortcomings will be addressed with the new
engines.

* Add test case for issue #276

Add Changelog entry

CHANGELOG.md

@@ -2,7 +2,7 @@
 
 All changes in this project will be noted in this file.
 
-## Version 0.8.0
+## Unreleased
 
 ### Additions
 
@@ -19,6 +19,12 @@ All changes in this project will be noted in this file.
   - Similary, all `inspect` queries have been changed
   - Entities are now of the form `space.model` instead of `ks:tbl`
 
+## Version 0.7.6
+
+### Fixes
+
+- Fixed erroneous removal of `auth` system table during tree cleanup (see #276)
+
 ## Version 0.7.5
 
 ### Additions

server/src/storage/v1/interface.rs

@@ -26,6 +26,8 @@
 
 //! Interfaces with the file system
 
+use std::collections::HashMap;
+
 use {
     crate::{
         corestore::memstore::Memstore,
@@ -86,57 +88,60 @@ pub fn create_tree_fresh<T: StorageTarget>(target: &T, memroot: &Memstore) -> Io
 /// throughout the lifecycle of the server
 pub fn cleanup_tree(memroot: &Memstore) -> IoResult<()> {
     if registry::get_cleanup_tripswitch().is_tripped() {
+        log::info!("We're cleaning up ...");
         // only run a cleanup if someone tripped the switch
         // hashset because the fs itself will not allow duplicate entries
         // the keyspaces directory will contain the PRELOAD file, but we'll just
         // remove it from the list
         let mut dir_keyspaces: HashSet<String> = read_dir_to_col!(DIR_KSROOT);
         dir_keyspaces.remove("PRELOAD");
-        let our_keyspaces: HashSet<String> = memroot
+        let our_keyspaces: HashMap<String, HashSet<String>> = memroot
             .keyspaces
             .iter()
-            .map(|kv| unsafe { kv.key().as_str() }.to_owned())
+            .map(|kv| {
+                let ksid = unsafe { kv.key().as_str() }.to_owned();
+                let tables: HashSet<String> = kv
+                    .value()
+                    .tables
+                    .iter()
+                    .map(|tbl| unsafe { tbl.key().as_str() }.to_owned())
+                    .collect();
+                (ksid, tables)
+            })
             .collect();
+
         // these are the folders that we need to remove; plonk the deleted keyspaces first
-        for folder in dir_keyspaces.difference(&our_keyspaces) {
+        let keyspaces_to_remove: Vec<&String> = dir_keyspaces
+            .iter()
+            .filter(|ksname| !our_keyspaces.contains_key(ksname.as_str()))
+            .collect();
+        for folder in keyspaces_to_remove {
             let ks_path = concat_str!(DIR_KSROOT, "/", folder);
             fs::remove_dir_all(ks_path)?;
         }
-        // now remove the tables
-        for keyspace in memroot.keyspaces.iter() {
-            let ks_path = unsafe { concat_str!(DIR_KSROOT, "/", keyspace.key().as_str()) };
+
+        // HACK(@ohsayan): Due to the nature of how system tables are stored in v1, we need to get rid of this
+        // ensuring that system tables don't end up being removed (since no system tables are actually
+        // purged at this time)
+        let mut our_keyspaces = our_keyspaces;
+        our_keyspaces.remove("system").unwrap();
+        let our_keyspaces = our_keyspaces;
+
+        // now remove the dropped tables
+        for (keyspace, tables) in our_keyspaces {
+            let ks_path = concat_str!(DIR_KSROOT, "/", keyspace.as_str());
+            // read what is present in the tables directory
             let mut dir_tbls: HashSet<String> = read_dir_to_col!(&ks_path);
             // in the list of directories we collected, remove PARTMAP because we should NOT
             // delete it
             dir_tbls.remove("PARTMAP");
-            let our_tbls: HashSet<String> = keyspace
-                .value()
-                .tables
-                .iter()
-                .map(|v| unsafe { v.key().as_str() }.to_owned())
-                .collect();
-            for old_file in dir_tbls.difference(&our_tbls) {
-                let fpath = concat_path!(&ks_path, old_file);
+            // find what tables we should remove
+            let tables_to_remove = dir_tbls.difference(&tables);
+            for removed_table in tables_to_remove {
+                let fpath = concat_path!(&ks_path, removed_table);
                 fs::remove_file(&fpath)?;
             }
         }
-        // now plonk the data files
-        for keyspace in memroot.keyspaces.iter() {
-            let ks_path = unsafe { concat_str!(DIR_KSROOT, "/", keyspace.key().as_str()) };
-            let dir_tbls: HashSet<String> = read_dir_to_col!(&ks_path);
-            let our_tbls: HashSet<String> = keyspace
-                .value()
-                .tables
-                .iter()
-                .map(|v| unsafe { v.key().as_str() }.to_owned())
-                .collect();
-            for old_file in dir_tbls.difference(&our_tbls) {
-                if old_file != "PARTMAP" {
-                    // plonk this data file; we don't need it anymore
-                    fs::remove_file(concat_path!(&ks_path, old_file))?;
-                }
-            }
-        }
     }
     Ok(())
 }

server/src/tests/issue_tests.rs

@@ -0,0 +1,52 @@
+/*
+ * Created on Fri Aug 12 2022
+ *
+ * This file is a part of Skytable
+ * Skytable (formerly known as TerrabaseDB or Skybase) is a free and open-source
+ * NoSQL database written by Sayan Nandan ("the Author") with the
+ * vision to provide flexibility in data modelling without compromising
+ * on performance, queryability or scalability.
+ *
+ * Copyright (c) 2022, Sayan Nandan <ohsayan@outlook.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <https://www.gnu.org/licenses/>.
+ *
+*/
+
+mod issue_276 {
+    // Refer to the issue here: https://github.com/skytable/skytable/issues/276
+    // Gist of issue: the auth table was cleaned up because the dummy ks in Memstore does not have
+    // the system tables
+    use skytable::{Element, Query, RespCode};
+    #[sky_macros::dbtest_func(port = 2005, auth_testuser = true, skip_if_cfg = "persist-suite")]
+    async fn first_run() {
+        // create the space
+        let r: Element = con
+            .run_query(Query::from("create space please_do_not_vanish"))
+            .await
+            .unwrap();
+        assert_eq!(r, Element::RespCode(RespCode::Okay));
+        // drop the space
+        let r: Element = con
+            .run_query(Query::from("drop space please_do_not_vanish"))
+            .await
+            .unwrap();
+        assert_eq!(r, Element::RespCode(RespCode::Okay));
+    }
+    #[sky_macros::dbtest_func(port = 2005, auth_testuser = true, run_if_cfg = "persist-suite")]
+    async fn second_run() {
+        // this function is just a dummy fn; if, as described in #276, the auth data is indeed
+        // lost, then the server will simply fail to start up
+    }
+}

server/src/tests/mod.rs

@@ -38,6 +38,7 @@ mod kvengine_list;
 mod persist;
 mod pipeline;
 mod snapshot;
+mod issue_tests;
 
 mod tls {
     use skytable::{query, Element};