zaino-state: Race conditions in `NodeBackedChainIndex::{status, shutdown}`

[str4d]

zaino/zaino-state/src/chain_index.rs

Lines 418 to 437 in fc4a511

	/// Shut down the sync process, for a cleaner drop
	/// an error indicates a failure to cleanly shutdown. Dropping the
	/// chain index should still stop everything
	pub async fn shutdown(&self) -> Result<(), FinalisedStateError> {
	self.finalized_db.shutdown().await?;
	self.mempool.close();
	self.status.store(StatusType::Closing as usize);
	Ok(())
	}
	/// Displays the status of the chain_index
	pub fn status(&self) -> StatusType {
	let finalized_status = self.finalized_db.status();
	let mempool_status = self.mempool.status();
	let combined_status = StatusType::from(self.status.load())
	.combine(finalized_status)
	.combine(mempool_status);
	self.status.store(combined_status as usize);
	combined_status
	}

I noticed a couple of issues here (there may be more):

• NodeBackedChainIndex::shutdown calls its inner shutdown methods, and only sets self.status to StatusType::Closing once they all return. This means that there is a window of time where a subscriber can check NodeBackedChainIndex::status and see the indexer is running, then try to perform some action while self.finalized_db is actually in a partially or fully shut down state.
  • There happens to be no race with any caller that executes after self.mempool.close() starts, because that method sets its internal status to StatusType::Closing first, and then NodeBackedChainIndex::status will take that as the most important status when combining. But we can't rely on that overall.
• If the self.status.store(StatusType::Closing) call inside NodeBackedChainIndex::shutdown were moved to the start of the method (to fix the above race condition), this would introduce a race condition between that and NodeBackedChainIndex::status, because the latter also calls self.status.store() and could overwrite the StatusType::Closing status, creating another window in which an external subscriber can observe the status of the indexer to be "running" when it is actually partially shut down.

[idky137]

I've been having a look into this today, I think we can remove the self.status.store() from NodeBackedChainIndex::status and move this logic into the sync loop itself, I think we should probably replace it with .combine(self.status.load()). This would enable us to move self.status.store(StatusType::Closing as usize) to that start of NodeBackedChainIndex::shutdown, but now im looking at the actual sync loop I think there may be a few other places we need to fix this.

The fix I have come up with is to add the following method to AtomicStatus and replace all instances of store(), except for pushing the shutdown signal, with this:

    /// Combine the current status with `new_status` atomically.
    /// If current is `Closing`, this is a no-op.
    /// Otherwise applies `StatusType::combine(current, new_status)` and commits via CAS.
    pub fn combine_store(&self, new_status: StatusType) {
        loop {
            let current_raw = self.0.load(Ordering::SeqCst);
            let current = StatusType::from(current_raw);

            // Closing is sticky; never update past it.
            if current == StatusType::Closing {
                return;
            }

            let next = current.combine(new_status) as usize;

            // If nothing changes, bail early.
            if next == current_raw {
                return;
            }

            match self
                .0
                .compare_exchange(current_raw, next, Ordering::SeqCst, Ordering::SeqCst)
            {
                Ok(_) => return,
                Err(_lost_race) => continue, // retry with fresh current
            }
        }
    }

I would be interested to see if you think this will be sufficient.

[nachog00]

I think we definitely want to make status a read only operation... it's currently doing too much, reading a writing.

But also, isn't this a possible issue when trying to combine values? are we counting on statuses not needing to be constantly consistent but eventually (i mean, on the following example, the critical error would be picked up on the next read...)?

  let finalized = self.finalized_db.status();    // Read at time T1
  let mempool = self.mempool.status();           // Read at time T2
  let current = self.status.load();             // Read at time T3

  // Components could change between T1, T2, and T3!

T1: finalized_db.status() → Ready
T2: [finalized_db encounters disk error, sets status to CriticalError]
T3: mempool.status() → Ready
T4: current.status() → Ready
T5: combine(Ready, Ready, Ready) → Ready ← WRONG! Should be CriticalError

The result: We report Ready even though a component is in CriticalError state.

[idky137]

Currently the status is written to in three places:

• initialisation and the the sync loop of the process that the status belongs to.
• the shutdown method (used to send shutdown signal to child processes.
• the status method ( this is bad and should have been removed! ).

My proposed solution is to restrict this to 2 places and provide a method that never overwrites a shutdown status:

• move current logic in status method into sync loop, status now just reads self.status.
• initialisation and sync loop use new safe method.
• shutdown logic stays the same.

We could restrict write to only the initialisation and sync loop but this would require us to separate the shutdown signal completely and would incur a small memory overhead.

It's worth noting that this shutdown logic is widely used so whichever solution we choose will need to be propagated throughout the codebase.

[idky137]

I think we definitely want to make status a read only operation... it's currently doing too much, reading a writing.

But also, isn't this a possible issue when trying to combine values? are we counting on statuses not needing to be constantly consistent but eventually (i mean, on the following example, the critical error would be picked up on the next read...)?

  let finalized = self.finalized_db.status();    // Read at time T1
  let mempool = self.mempool.status();           // Read at time T2
  let current = self.status.load();             // Read at time T3

  // Components could change between T1, T2, and T3!

T1: finalized_db.status() → Ready
T2: [finalized_db encounters disk error, sets status to CriticalError]
T3: mempool.status() → Ready
T4: current.status() → Ready
T5: combine(Ready, Ready, Ready) → Ready ← WRONG! Should be CriticalError

The result: We report Ready even though a component is in CriticalError state.

I think this is more just an issue of realtime systems. The status may change moments after you poll the system, and with latency this could even be before the status is received.

We could add a combine method that checks for change but it would not solve this problem as the status could still change the moment we return the it.

[nachog00]

Currently the status is written to in three places:

• initialisation and the the sync loop of the process that the status belongs to.
• the shutdown method (used to send shutdown signal to child processes.
• the status method ( this is bad and should have been removed! ).

My proposed solution is to restrict this to 2 places and provide a method that never overwrites a shutdown status:

• move current logic in status method into sync loop, status now just reads self.status.
• initialisation and sync loop use new safe method.
• shutdown logic stays the same.

We could restrict write to only the initialisation and sync loop but this would require us to separate the shutdown signal completely and would incur a small memory overhead.

It's worth noting that this shutdown logic is widely used so whichever solution we choose will need to be propagated throughout the codebase.

I also considered a dedicated shutdown_signal atomic_boolean. That one we could atomically swap and better ensure it never goes backwards... I think there are ways to implement that with minimal changes to the AtomicStatus struct api if we decide to explore that option.

[idky137]

Currently the status is written to in three places:

• initialisation and the the sync loop of the process that the status belongs to.
• the shutdown method (used to send shutdown signal to child processes.
• the status method ( this is bad and should have been removed! ).

My proposed solution is to restrict this to 2 places and provide a method that never overwrites a shutdown status:

• move current logic in status method into sync loop, status now just reads self.status.
• initialisation and sync loop use new safe method.
• shutdown logic stays the same.

We could restrict write to only the initialisation and sync loop but this would require us to separate the shutdown signal completely and would incur a small memory overhead.
It's worth noting that this shutdown logic is widely used so whichever solution we choose will need to be propagated throughout the codebase.

I also considered a dedicated shutdown_signal atomic_boolean. That one we could atomically swap and better ensure it never goes backwards... I think there are ways to implement that with minimal changes to the AtomicStatus struct api if we decide to explore that option.

Ye this is something I have considered switching to in the past, a single "online / running" AtomicBool might be a nice way to handle this.

If we do go this route we would probably want this to be an input variable to the spawn methods in zaino-state so it can be controlled by the external process (in our case zainod). Maybe this should be optional so, if for instance Zallet wants this to be completely separate from their control logic they can just spawn an IndexerService without this input and just use the shutdown method, but we can still have more fine grained control in zainod.