GH-1426: Improve handling of Broker data store creation failures

Broker::create_master() and Broker::create_clone() now return
a valid value even when there's a failure to open the backend database
(e.g. SQLite filesystem error).  In that case, the returned value can
still be passed into other data store operations, but they'll fail
immediately with an error.  Broker::is_closed() can now also be used to
determine whether the data store creation calls failed.

auxil/broker

@@ -1 +1 @@
-Subproject commit b9f68a1ddebe82db28068c76153b0954c1f7c227
+Subproject commit c8d9a8fb1ce768cfbcf81767a34a15ffd8104d0d

scripts/base/frameworks/broker/store.zeek

@@ -78,7 +78,9 @@ export {
 	##
 	## options: tunes how some storage backends operate.
 	##
-	## Returns: a handle to the data store.
+	## Returns: a handle to the data store for which a subsequent call to
+	##          :zeek:see:`Broker::is_closed` will return true if the store
+	##          could not be created/opened.
 	global create_master: function(name: string, b: BackendType &default = MEMORY,
 	                               options: BackendOptions &default = BackendOptions()): opaque of Broker::Store;
 
@@ -111,7 +113,9 @@ export {
 	##                           acknowledged by the master.  A negative/zero
 	##                           value indicates that commands never buffer.
 	##
-	## Returns: a handle to the data store.
+	## Returns: a handle to the data store for which a subsequent call to
+	##          :zeek:see:`Broker::is_closed` will return true if the store
+	##          could not be created/opened.
 	global create_clone: function(name: string,
 	                              resync_interval: interval &default = default_clone_resync_interval,
 	                              stale_interval: interval &default = default_clone_stale_interval,

src/broker/Manager.cc

@@ -1746,6 +1746,10 @@ bool Manager::CloseStore(const string& name)
 		++i;
 		}
 
+	s->second->have_store = false;
+	s->second->store_pid = {};
+	s->second->proxy = {};
+	s->second->store = {};
 	Unref(s->second);
 	data_stores.erase(s);
 	return true;

src/broker/Store.h

@@ -114,8 +114,12 @@ private:
  */
 class StoreHandleVal : public OpaqueVal {
 public:
+	StoreHandleVal()
+		: OpaqueVal(Broker::detail::opaque_of_store_handle)
+		{}
+
 	StoreHandleVal(broker::store s)
-		: OpaqueVal(Broker::detail::opaque_of_store_handle), store{s}, proxy{store}, store_pid{store.frontend_id()}
+		: OpaqueVal(Broker::detail::opaque_of_store_handle), store{std::move(s)}, proxy{store}, store_pid{store.frontend_id()}, forward_to{}, have_store{true}
 		{ }
 
 	void ValDescribe(ODesc* d) const override;
@@ -125,16 +129,13 @@ public:
 	broker::publisher_id store_pid;
 	// Zeek table that events are forwarded to.
 	TableValPtr forward_to;
+	bool have_store = false;
 
 protected:
 
 	IntrusivePtr<Val> DoClone(CloneState* state) override
 		{ return { NewRef{}, this }; }
 
-	StoreHandleVal()
-		: OpaqueVal(Broker::detail::opaque_of_store_handle)
-		{}
-
 	DECLARE_OPAQUE_VALUE(StoreHandleVal)
 };
 
@@ -145,4 +146,4 @@ broker::backend to_backend_type(BifEnum::Broker::BackendType type);
 broker::backend_options to_backend_options(broker::backend backend,
                                            RecordVal* options);
 
-} // namespace zeek::Broker
+} // namespace zeek::Broker::detail

src/broker/store.bif

@@ -8,7 +8,10 @@
 #include "zeek/Trigger.h"
 
 static zeek::Broker::detail::StoreHandleVal* to_store_handle(zeek::Val* h)
-	{ return dynamic_cast<zeek::Broker::detail::StoreHandleVal*>(h); }
+	{
+	auto rval = dynamic_cast<zeek::Broker::detail::StoreHandleVal*>(h);
+	return rval && rval->have_store ? rval : nullptr;
+	}
 %%}
 
 module Broker;
@@ -41,7 +44,7 @@ function Broker::__create_master%(id: string, b: BackendType,
 	if ( ! store )
 		{
 		zeek::emit_builtin_error(zeek::util::fmt("Could not create Broker master store '%s'", name));
-		return nullptr;
+		return make_intrusive<zeek::Broker::detail::StoreHandleVal>();
 		}
 
 	return store;
@@ -66,7 +69,7 @@ function Broker::__create_clone%(id: string, resync_interval: interval,
 	if ( ! store )
 		{
 		zeek::emit_builtin_error(zeek::util::fmt("Could not create clone of Broker store '%s'", name));
-		return nullptr;
+		return make_intrusive<zeek::Broker::detail::StoreHandleVal>();
 		}
 
 	return store;
@@ -75,12 +78,15 @@ function Broker::__create_clone%(id: string, resync_interval: interval,
 function Broker::__is_closed%(h: opaque of Broker::Store%): bool
 	%{
 	zeek::Broker::Manager::ScriptScopeGuard ssg;
-	auto handle = to_store_handle(h);
+	auto handle = dynamic_cast<zeek::Broker::detail::StoreHandleVal*>(h);
 
 	if ( ! handle )
 		zeek::detail::emit_builtin_exception("invalid Broker store handle", h);
 
-	return zeek::val_mgr->Bool(broker_mgr->LookupStore(handle->store.name()));
+	if ( ! handle->have_store )
+		return zeek::val_mgr->True();
+
+	return zeek::val_mgr->Bool(! broker_mgr->LookupStore(handle->store.name()));
 	%}
 
 function Broker::__close%(h: opaque of Broker::Store%): bool

testing/btest/Baseline/broker.store.create-failure/zeek.err

@@ -0,0 +1,11 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+error in <...>/create-failure.zeek, line 63: Failed to attach master store backend_failure: (Broker::create_master(../fail, Broker::SQLITE, (coerce [] to Broker::BackendOptions)))
+error in <...>/create-failure.zeek, line 63: Could not create Broker master store '../fail' (Broker::create_master(../fail, Broker::SQLITE, (coerce [] to Broker::BackendOptions)))
+error in <...>/create-failure.zeek, line 49: invalid Broker store handle (Broker::keys(s) and broker::store::{})
+error in <...>/create-failure.zeek, line 27: invalid Broker store handle (Broker::close(m1) and broker::store::{})
+error in <...>/create-failure.zeek, line 33: invalid Broker store handle (Broker::close(c2) and broker::store::{})
+error in <...>/create-failure.zeek, line 49: invalid Broker store handle (Broker::keys(s) and broker::store::{})
+error in <...>/create-failure.zeek, line 49: invalid Broker store handle (Broker::keys(s) and broker::store::{})
+error in <...>/create-failure.zeek, line 49: invalid Broker store handle (Broker::keys(s) and broker::store::{})
+error in <...>/create-failure.zeek, line 49: invalid Broker store handle (Broker::keys(s) and broker::store::{})
+received termination signal

testing/btest/Baseline/broker.store.create-failure/zeek.out

@@ -0,0 +1,21 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+T
+F
+F
+F
+m1 keys result: [status=Broker::FAILURE, result=[data=<uninitialized>]]
+m2 keys result: [status=Broker::SUCCESS, result=[data=broker::data{{}}]]
+c2 keys result: [status=Broker::SUCCESS, result=[data=broker::data{{}}]]
+T
+F
+F
+F
+T
+T
+T
+T
+m1 keys result: [status=Broker::FAILURE, result=[data=<uninitialized>]]
+c1 keys result: [status=Broker::FAILURE, result=[data=<uninitialized>]]
+m2 keys result: [status=Broker::FAILURE, result=[data=<uninitialized>]]
+c2 keys result: [status=Broker::FAILURE, result=[data=<uninitialized>]]
+c1 timeout

testing/btest/broker/store/create-failure.zeek

@@ -0,0 +1,77 @@
+# @TEST-EXEC: mkdir fail.sqlite
+# @TEST-EXEC: btest-bg-run zeek "BROKER_FILE_VERBOSITY=error BROKER_CONSOLE_VERBOSITY=quiet zeek -b %INPUT >out 2>err"
+# @TEST-EXEC: btest-bg-wait 60
+# @TEST-EXEC: btest-diff zeek/out
+# @TEST-EXEC: TEST_DIFF_CANONIFIER="$SCRIPTS/diff-remove-abspath $SCRIPTS/diff-sort" btest-diff zeek/err
+
+redef exit_only_after_terminate = T;
+
+global c = 0;
+global m1: opaque of Broker::Store;
+global m2: opaque of Broker::Store;
+global c1: opaque of Broker::Store;
+global c2: opaque of Broker::Store;
+
+global check_it: function(name: string, s: opaque of Broker::Store);
+
+function check_terminate_conditions()
+	{
+	++c;
+	if ( c == 4 )
+		{
+		print Broker::is_closed(m1);
+		print Broker::is_closed(m2);
+		print Broker::is_closed(c1);
+		print Broker::is_closed(c2);
+		# Failed to originally open m1, so should raise an error.
+		Broker::close(m1);
+		Broker::close(m2);
+		Broker::close(c1);
+		# Closing c2 is an error since it's actually referring to m2, already
+		# closed above (i.e. making a clone of a master store that already lives
+		# in the same process, just returns a handle to the master).
+		Broker::close(c2);
+		print Broker::is_closed(m1);
+		print Broker::is_closed(m2);
+		print Broker::is_closed(c1);
+		print Broker::is_closed(c2);
+		check_it("m1", m1);
+		check_it("c1", c1);
+		check_it("m2", m2);
+		check_it("c2", c2);
+		}
+	else if ( c == 8 )
+		terminate();
+	}
+
+function check_it(name: string, s: opaque of Broker::Store)
+	{
+	when ( local r = Broker::keys(s) )
+		{
+		check_terminate_conditions();
+		print fmt("%s keys result: %s", name, r);
+		}
+	timeout 1sec
+		{
+		check_terminate_conditions();
+		print fmt("%s timeout", name);
+		}
+	}
+
+event zeek_init()
+	{
+	m1 = Broker::create_master("../fail", Broker::SQLITE);
+	m2 = Broker::create_master("ok");
+	c1 = Broker::create_clone("../fail");
+	c2 = Broker::create_clone("ok");
+
+	print Broker::is_closed(m1);
+	print Broker::is_closed(m2);
+	print Broker::is_closed(c1);
+	print Broker::is_closed(c2);
+
+	check_it("m1", m1);
+	check_it("c1", c1);
+	check_it("m2", m2);
+	check_it("c2", c2);
+	}