chore: ntx builder actor deactivation#1705
chore: ntx builder actor deactivation#1705SantiagoPittella wants to merge 6 commits intosantiagopittella-ntx-code-organizationfrom
Conversation
crates/ntx-builder/src/actor/mod.rs
Outdated
| // Sterility timeout: actor has been idle too long, request shutdown. | ||
| _ = sterility_sleep => { | ||
| match self.initiate_shutdown(account_id).await { | ||
| Ok(()) => return ActorShutdownReason::Sterile(account_id), | ||
| Err(()) => self.mode = ActorMode::NotesAvailable, | ||
| } | ||
| } |
There was a problem hiding this comment.
I understand that this is following the established pattern, but I think we're needlessly complicating matters.
If the actor wants to exit, just exit the task. We don't actually need a shutdown handle or token either.
ActorShutdownReason can be replaced with a conventional -> Result<(), Error>. This may complicate this PR, so I'm okay with simply returning, and cleaning that up separately.
| // Sterility timeout: actor has been idle too long, request shutdown. | |
| _ = sterility_sleep => { | |
| match self.initiate_shutdown(account_id).await { | |
| Ok(()) => return ActorShutdownReason::Sterile(account_id), | |
| Err(()) => self.mode = ActorMode::NotesAvailable, | |
| } | |
| } | |
| // Sterility timeout: actor has been idle too long, deactivate account | |
| _ = sterility_sleep => return ActorShutdownReason::Sterile, |
| /// Handles a shutdown request from an actor that has been idle for longer than the sterility | ||
| /// timeout. | ||
| /// | ||
| /// Validates the request by checking the DB for available notes. If notes are available, the | ||
| /// shutdown is rejected by dropping `ack_tx` (the actor detects the `RecvError` and resumes). | ||
| /// If no notes are available, the actor is deregistered and the ack is sent, allowing the | ||
| /// actor to exit gracefully. | ||
| pub async fn handle_shutdown_request( | ||
| &mut self, | ||
| account_id: NetworkAccountId, | ||
| block_num: BlockNumber, | ||
| max_note_attempts: usize, | ||
| ack_tx: tokio::sync::oneshot::Sender<()>, | ||
| ) { | ||
| let has_notes = self | ||
| .db | ||
| .has_available_notes(account_id, block_num, max_note_attempts) | ||
| .await | ||
| .unwrap_or(false); | ||
|
|
||
| if has_notes { | ||
| // Reject: drop ack_tx -> actor detects RecvError, resumes. | ||
| tracing::debug!( | ||
| %account_id, | ||
| "Rejected actor shutdown: notes available in DB" | ||
| ); | ||
| } else { | ||
| self.actor_registry.remove(&account_id); | ||
| let _ = ack_tx.send(()); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I would avoid this entirely. Let the actor deactivate itself.
In the select! arm that reaps actor tasks, we can then check if the deactivated actor should be restarted by inspecting that actor's notify channel (which we have a copy of of here). If there is a pending notification, then we know the actor didn't handle it.
This should be rare because it implies the actor idle timed out just as we notified it. And therefore, even though this code is technically more optimal because the actor doesn't have to respawn, I wouldn't be concerned about the performance angle.
There was a problem hiding this comment.
Ok, sounds good. I will address this with the previous comment too
e8864fa to
712ece2
Compare
c30c953 to
d56b50b
Compare
712ece2 to
acacbf3
Compare
d56b50b to
ac9086b
Compare
Closes the third task of #1694