[RSDK-8948] refactor task lifecycles for graceful shutdown #503

gvaradarajan · 2025-05-09T21:31:09Z

No description provided.

micro-rdk/src/common/restart_monitor.rs

micro-rdk/src/common/conn/errors.rs

micro-rdk/src/common/system.rs

micro-rdk/src/common/conn/viam.rs

mattjperez · 2025-05-12T13:17:24Z

micro-rdk/src/common/conn/viam.rs


-        let mut tasks: FuturesUnordered<_> = FuturesUnordered::new();


mattjperez · 2025-05-12T13:19:32Z

micro-rdk/src/common/conn/viam.rs

-        log::info!("viam server started");
-        while let Some(ret) = tasks.next().await {
-            log::error!("task ran returned {:?}", ret);
+        let (app_client_tasks_result, server_task_result) = system_task.await;


that's pretty cool how that works

micro-rdk/src/common/conn/viam.rs

mattjperez

LGTM, however might want to wait for the poll function in viam.rs to get a second reviewer

npmenard

Are we running the desctructor of LocalRobot before we shutdown ViamServer?

gvaradarajan · 2025-05-15T19:24:57Z

Are we running the desctructor of LocalRobot before we shutdown ViamServer?

we are now

npmenard

LGTM left a couple of comments / nits
I did some quick test and I always ran into the timeout (120 secs) am I doing something wrong?

npmenard · 2025-05-16T16:49:42Z

micro-rdk/src/common/conn/viam.rs

+            }
+        }
+        exec.block_on(force_shutdown(run_result.app_client.clone()));
+        unreachable!()


nit: so this doesn't get us back to main?

Not sure how I would do that besides putting the shutdown call in main.rs (which doesn't seem like a good idea?)

npmenard · 2025-05-16T16:52:21Z

micro-rdk/src/common/conn/viam.rs

@@ -1307,6 +1447,9 @@ mod tests {
                "/viam.app.v1.RobotService/Log" => self.log(body.split_off(5)),
                "/viam.app.v1.RobotService/NeedsRestart" => self.needs_restart(body.split_off(5)),
                "/viam.app.v1.RobotService/Config" => self.get_config(),
+                "/viam.app.agent.v1.AgentDeviceService/DeviceAgentConfig" => {
+                    return Err(ServerError::new(GrpcError::RpcInternal, None));


return unimplemented by default?

Just to clarify, do you mean that this shouldn't panic, or that I should just change the error for this endpoint to unimplemented?

npmenard · 2025-05-16T16:53:51Z

micro-rdk/src/common/system.rs

+        match self {
+            Self::Restart => "restart".to_string(),
+            Self::DeepSleep(dur) => dur.as_ref().map_or("enter deep sleep".to_string(), |d| {
+                format!("enter deep sleep for {} microseconds", d.as_micros())


how would you go about configuring other wake up sources?

was that part of the scope of this? If so I can add that in a follow-up PR where I change the DeepSleep enum variant to be a struct with all of the possible settings. Then you would configure these settings through the agent config (just as you would the active timeout)

nope just a general question

npmenard · 2025-05-16T16:54:33Z

micro-rdk/src/common/system.rs

+}
+
+impl SystemEvent {
+    fn action_string(&self) -> String {


should implement Display trait

gvaradarajan · 2025-05-16T18:59:08Z

LGTM left a couple of comments / nits I did some quick test and I always ran into the timeout (120 secs) am I doing something wrong?

@npmenard you're not doing anything wrong, it's just that the server task waits for a connection for much longer than the timeout. Wasn't sure exactly how to interrupt that, but I can give it another shot

npmenard · 2025-05-19T12:31:16Z

LGTM left a couple of comments / nits I did some quick test and I always ran into the timeout (120 secs) am I doing something wrong?

@npmenard you're not doing anything wrong, it's just that the server task waits for a connection for much longer than the timeout. Wasn't sure exactly how to interrupt that, but I can give it another shot

yes we shouldn't wait more than needed (task done, client close etc...)

This reverts commit 62feb34.

acmorrow

I took first pass through, here are some comments - mostly questions. I'll probably take another look on Tuesday morning to try to understand a few parts better.

acmorrow · 2025-05-23T17:12:57Z

micro-rdk/src/common/webrtc/api.rs

@@ -545,8 +575,8 @@ pub struct WebRtcApi<C, E> {

 impl<C, E> WebRtcApi<C, E>
 where
-    C: Certificate,
-    E: WebRtcExecutor<Pin<Box<dyn Future<Output = ()>>>> + Clone,
+    C: Certificate + 'static,


Just curious, why does this need 'static now?

I was trying something that didn't work out and I forgot to remove this

acmorrow · 2025-05-23T17:16:05Z

micro-rdk/src/common/system.rs

+use super::log::LogUploadTask;
+use super::runtime::terminate;
+
+pub(crate) static SHUTDOWN_EVENT: LazyLock<Arc<AsyncMutex<Option<SystemEvent>>>> =


Can we leave a comment that, should we find ourselves back here, it would be a good idea to find a way to make this not global? I don't think it needs a ticket, just a TODO: Find a way to do this without introducing mutable global state.

acmorrow · 2025-05-23T17:18:08Z

micro-rdk/src/common/system.rs

+    }
+}
+
+pub(crate) async fn send_system_change(event: SystemEvent) {


send_system_event?

Also,. just thinking aloud here, but this makes me wonder if eventually this should be a channel, rather than just a slot. For now, a slot makes sense because both system events are terminal, but I could see a use for a more general facility later.

acmorrow · 2025-05-23T17:18:48Z

micro-rdk/src/common/system.rs

+use super::log::LogUploadTask;
+use super::runtime::terminate;
+
+pub(crate) static SHUTDOWN_EVENT: LazyLock<Arc<AsyncMutex<Option<SystemEvent>>>> =


Does SHUTDOWN_EVENT need to be pub at all? I think I'd be happier if it were entirely an implementation detail used by the API here.

acmorrow · 2025-05-23T17:19:42Z

micro-rdk/src/common/system.rs

+}
+
+pub(crate) async fn force_shutdown(app_client: Option<AppClient>) {
+    // flush logs


Should there be a deadline on this? Or is there already, implicitly, elsewhere.

There is not, I will add one

acmorrow · 2025-05-23T17:20:36Z

micro-rdk/src/common/system.rs

+                    if result != crate::esp32::esp_idf_svc::sys::ESP_OK {
+                        unreachable!("duration requested too long")
+                    }
+                    log::warn!("Esp32 entering deep sleep for {} microseconds!", dur_micros);


Will this log ever be seen?

it will be seen in the monitor

acmorrow · 2025-05-23T17:21:29Z

micro-rdk/src/common/system.rs

+                async_io::block_on(Timer::after(dur));
+                terminate();
+            } else {
+                log::error!("sleep from wake up not supported for native builds")


The message needs a little cleanup I think. Also, is it meaningful for execution to continue from here?

It was either this or panic I feel, do we prefer panic?

acmorrow · 2025-05-23T17:30:48Z

micro-rdk/src/common/conn/viam.rs

                let mut app_client_tasks: FuturesUnordered<AppClientTaskRunner> =
                    FuturesUnordered::new();
                log::info!("starting execution of app client tasks");
                for task in &self.app_client_tasks {
+                    log::info!("queueing {:?} task", task.name());


Maybe debug?

meant to remove this entirely actually

acmorrow · 2025-05-23T17:34:20Z

micro-rdk/src/common/conn/viam.rs

@@ -777,16 +884,26 @@ where
                        },
                    });
                }
+                let shutdown_was_requested = shutdown_requested_nonblocking().await;


It looks to me like this only checks shutdown before it starts running the app client tasks. Shouldn't it be being awaited in parallel with the app client tasks, so that if shutdown is requested the app client tasks stop? This is probably me misunderstanding, so, just looking for a little more explanation here. Which maybe should get folded into a comment.

That logic is being handled in TaskRunnerState and AppClientTaskRunner. I'll add some comments

npmenard

LGTM left a couple of comments. Mainly concerned by the increased stack usage

npmenard · 2025-05-26T23:42:06Z

micro-rdk/src/common/conn/viam.rs

-            if let Err(e) = self.serve_incoming_connection(incoming).await {
-                log::error!("failed to serve incoming connection: {:?}", e)
+
+            let mut connection_future =


does this have a penalty on stack consumption vs how it was before?

not sure, I'll profile the stack usage and get back to you on that

There is a penalty of about 1.5 KB of stack, do we feel this is unacceptable?

Resolved offline, this is deemed as "not great, but temporarily acceptable"

npmenard · 2025-05-27T00:45:05Z

micro-rdk/src/common/webrtc/api.rs

-                // silence errors coming from context cancellation
-                if *code == 1 {
-                    return Ok(None);
+            let mut shutdown_poll_future = Box::pin(


same question about stack

npmenard · 2025-05-27T14:10:39Z

micro-rdk/src/common/conn/server.rs

@@ -59,6 +60,10 @@ impl WebRTCConnection {
    }
    pub(crate) async fn run(&mut self) -> Result<(), ServerError> {
        loop {
+            if shutdown_requested_nonblocking().await {


should we just unilaterally close connections? this basically does that already. We might want to support graceful shutdown of `DTLS/HTTP2 connections later

resolved offline

npmenard · 2025-05-27T14:12:35Z

micro-rdk/src/common/conn/viam.rs

+                    return Ok(());
+                } else {
+                    log::error!("reached state of completed tasks without shutdown request, additional investigation required");
+                    return Err(errors::ServerError::ServerInvalidCompletedState);


Can that even happen?

I don't think so. But if it does, it's wrong so I thought we'd want to make sure we know without necessarily producing a coredump

npmenard · 2025-05-27T14:12:59Z

micro-rdk/src/common/conn/viam.rs

@@ -1307,6 +1447,9 @@ mod tests {
                "/viam.app.v1.RobotService/Log" => self.log(body.split_off(5)),
                "/viam.app.v1.RobotService/NeedsRestart" => self.needs_restart(body.split_off(5)),
                "/viam.app.v1.RobotService/Config" => self.get_config(),
+                "/viam.app.agent.v1.AgentDeviceService/DeviceAgentConfig" => {
+                    return Err(ServerError::new(GrpcError::RpcInternal, None));


npmenard · 2025-05-27T14:14:04Z

micro-rdk/src/common/system.rs

+
+pub(crate) async fn send_system_change(event: SystemEvent) {
+    log::info!("received call to {}", event);
+    let _ = SHUTDOWN_EVENT.lock().await.insert(event);


if we have multiple event they are going to be overwritten no?

I was thinking that we actually want to respect the latest requested event?

never mind, that decision's ridiculous. I'll fix this

npmenard · 2025-05-27T14:14:45Z

micro-rdk/src/common/system.rs

+            }
+        }
+        None => {
+            log::error!("call to shutdown/restart without request to system, restarting");


nit: terminating.

gvaradarajan requested a review from a team as a code owner May 9, 2025 21:31

mattjperez reviewed May 12, 2025

View reviewed changes

gvaradarajan requested review from acmorrow, npmenard and mattjperez May 13, 2025 13:00

mattjperez approved these changes May 14, 2025

View reviewed changes

npmenard reviewed May 15, 2025

View reviewed changes

gvaradarajan requested a review from npmenard May 15, 2025 19:27

npmenard approved these changes May 16, 2025

View reviewed changes

gvaradarajan added 10 commits May 22, 2025 14:26

[RSDK-8948] refactor task lifecycles for graceful shutdown

f3d0c7f

rm stray comment

e1d8b5b

address comments and add sleep sim for native

4e3905d

remove simulation

c9d0f8b

fixed bug blocking server task

9febbd5

Revert "remove simulation"

9f15134

This reverts commit 62feb34.

the actual fix

eebb2c9

import

9979416

meaningfully return from run so robot gets dropped

ec114db

fix signaling and server tasks not properly cancelling

8f72099

gvaradarajan force-pushed the RSDK-8948-system-lifecycle branch from d0094f0 to 8f72099 Compare May 22, 2025 18:36

impl Display

19779ed

gvaradarajan requested a review from npmenard May 22, 2025 20:42

Merge branch 'main' into RSDK-8948-system-lifecycle

676fc19

acmorrow reviewed May 23, 2025

View reviewed changes

npmenard approved these changes May 27, 2025

View reviewed changes

gvaradarajan added 2 commits May 27, 2025 11:34

address some review somments

626ea42

clarifying comment

89946c2

Merge branch 'main' into RSDK-8948-system-lifecycle

a7b9e02

gvaradarajan merged commit 63803aa into viamrobotics:main May 27, 2025
6 checks passed


		let mut tasks: FuturesUnordered<_> = FuturesUnordered::new();

[RSDK-8948] refactor task lifecycles for graceful shutdown #503

[RSDK-8948] refactor task lifecycles for graceful shutdown #503

Uh oh!

Conversation

gvaradarajan commented May 9, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattjperez left a comment

Choose a reason for hiding this comment

Uh oh!

npmenard left a comment

Choose a reason for hiding this comment

Uh oh!

gvaradarajan commented May 15, 2025

Uh oh!

npmenard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gvaradarajan commented May 16, 2025

Uh oh!

npmenard commented May 19, 2025

Uh oh!

acmorrow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!