| Message ID | 72ec038f-df3f-4ad2-9c73-8241eab4b97en@googlegroups.com |
|---|---|
| State | New |
| Headers | show |
| Series | install_from_file: Fix silent failure and add retries for post-update IPC | expand |
Hi Arturs, On 9/22/26 15:08, 'Arturs Laizans' via swupdate wrote: > From 77584022e56876c476c318f1dcb14fc3c43bba3c Mon Sep 17 00:00:00 2001 > From: Arturs Laizans <arturs.laizans.ext@kiongroup.com> > Date: Tue, 22 Sep 2026 14:51:53 +0200 > Subject: [PATCH] install_from_file: Fix silent failure and add retries for > post-update IPC > > When install_from_file() completes an installation successfully, > endupdate() issues an ipc_postupdate() call to execute any post-update > actions. However, if ipc_postupdate() fails (due to a transient socket > connection error, busy IPC listener, or context-switch delay) or if > the server responds with NACK, endupdate() silently overrides end_status > to EXIT_FAILURE without logging any error message. Let's start with the intention: install_from_file() is a backward compatible function, but it is not anymore how SWUpdate is designed to get the SWU. SWUpdate accepts streams, and in case of local files, they should be sent via own application or swupdate-client utility. The ipc_postupdate() function is thought to start something *AFTER* the update - this is wanted for cases where users require to clean up something, add some post processing, but it does not change the status of the update. The atomicity of the update is guaranteed until "SWUpdate was successful !", after that post update scripts or whatever can fail and they are not tracked. So first: same logic and code is implemented in core/install_from_file.c and in tools/swupdate-client.c. Both should be touched because it is the same use case. I could also remove at all the "-i" option in future because this is not part of the design I did, but it was set at the beginning for easy testing. For design, swupdate is a daemon waiting for incoming streams. > > As a result, users and frontends see: > [INFO ] : SWUPDATE running : [install_from_file] : SWUpdate was > successful ! > immediately followed by the process exiting with status 1 and no > indication of why the update was considered failed. I agree that status of the update shouldn't be changed. > > Furthermore, unlike the start of install_from_file() which retries > swupdate_async_start() up to 3 times with a 1-second delay to handle > IPC listener readiness, endupdate() previously attempted ipc_postupdate() Understood, this is a sort of race because the daemon is not really started, and the client is part of the daemon itself, as said, just for compatibility reason. But maybe it make smore sense to remove this code at all. > only once without retries, making it susceptible to transient socket races > under high load. I do not think there is high load, I guess there is a race because SWUpdate has finished and it is exiting, but in the same time some more work is issued (ipc_ostupdate). Do you experience the same issue if you use "swupdate-client <filename>" instead of "swupdate -i" ? > > Add retry logic (up to 3 attempts with 1-second backoff) for transient > IPC communication errors in endupdate(), and explicitly report errors > via ERROR() when post-update actions or IPC fail. ERROR is sent in cases where the Update itself must be considered failed. The update was successful, a post update something couldn't be executed with success, but this does not change the status of the update itself. Best regards, Stefano Babic > > Signed-off-by: Arturs Laizans <arturs.laizans.ext@kiongroup.com> > --- > core/install_from_file.c | 22 ++++++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/core/install_from_file.c b/core/install_from_file.c > index e222e30e..719f613e 100644 > --- a/core/install_from_file.c > +++ b/core/install_from_file.c > @@ -58,8 +58,26 @@ static int endupdate(RECOVERY_STATUS status) > > if (status == SUCCESS) { > ipc_message msg; > -msg.data.procmsg.len = 0; > -if (ipc_postupdate(&msg) != 0 || msg.type != ACK) { > +int ret = -1; > +int retries = 3; > + > +while (retries > 0) { > +memset(&msg, 0, sizeof(msg)); > +msg.data.procmsg.len = 0; > +ret = ipc_postupdate(&msg); > +if (ret == 0) > +break; > +retries--; > +if (retries > 0) > +sleep(1); > +} > + > +if (ret != 0) { > +ERROR("Post-update actions failed: IPC communication error"); > +end_status = EXIT_FAILURE; > +} else if (msg.type != ACK) { > +ERROR("Post-update actions failed: %s", > + msg.data.msg[0] ? msg.data.msg : "rejected by server"); > end_status = EXIT_FAILURE; > } > } > -- > 2.43.0 > > -- > You received this message because you are subscribed to the Google > Groups "swupdate" group. > To unsubscribe from this group and stop receiving emails from it, send > an email to swupdate+unsubscribe@googlegroups.com > <mailto:swupdate+unsubscribe@googlegroups.com>. > To view this discussion visit https://groups.google.com/d/msgid/ > swupdate/72ec038f-df3f-4ad2-9c73-8241eab4b97en%40googlegroups.com > <https://groups.google.com/d/msgid/swupdate/72ec038f- > df3f-4ad2-9c73-8241eab4b97en%40googlegroups.com? > utm_medium=email&utm_source=footer>.
Hi Stefano, Thank you for the quick and clear feedback! > Do you experience the same issue if you use "swupdate-client <filename>" instead of "swupdate -i" ? In our embedded setup, SWUpdate is invoked by an updater daemon on-demand to install local/nested SWU packages on subcomponents, so running standalone "swupdate -i" was used rather than having a persistent background daemon running. Looking at tools/swupdate-client.c, line 119 also has: end_status = EXIT_FAILURE; so swupdate-client exhibits the exact same behavior if post-update actions fail or race. Your architectural rationale makes total sense: the update itself is atomic and complete once "SWUpdate was successful !" is reached, and post-update actions are best-effort post-processing that should not invalidate the update. In v2: - Both core/install_from_file.c and tools/swupdate-client.c are updated so that end_status is preserved as EXIT_SUCCESS if the update succeeded. - Used WARN() instead of ERROR() in install_from_file.c so it does not indicate that the update failed. - Dropped the retry loop since post-update failures are no longer fatal. (Note: we noticed corelib/downloader.c:84 also sets result = FAILURE on ipc_postupdate failure; please let me know if you would like that aligned in this patch series as well.) Best regards, Arturs --- From 9cb78d5e1af9fd0c353e8241c4ce0181978d76da Mon Sep 17 00:00:00 2001 From: Arturs Laizans <arturs.laizans.ext@kiongroup.com> Date: Tue, 22 Sep 2026 16:36:26 +0200 Subject: [PATCH v2] install_from_file, swupdate-client: Do not fail update on post-update failure The update itself is atomic and complete once "SWUpdate was successful !" is reached. Post-update actions initiated via ipc_postupdate() are intended for subsequent actions or cleanup, but failure of post-update actions should not invalidate the already successful update or alter its final exit status. Previously, both install_from_file() and swupdate-client's end() callback overrode end_status to EXIT_FAILURE if ipc_postupdate() failed or returned NACK. In install_from_file(), this occurred silently without any error or warning output. Do not alter end_status on post-update failures in either install_from_file() or swupdate-client. Additionally, log a warning via WARN() in install_from_file() if post-update actions fail, so that issues with post-processing are visible without marking the update as failed. Signed-off-by: Arturs Laizans <arturs.laizans.ext@kiongroup.com> --- Changes in v2: - Do not override end_status to EXIT_FAILURE on post-update failures (update was already successful). - Apply the fix consistently across both core/install_from_file.c and tools/swupdate-client.c. - Use WARN() instead of ERROR() in install_from_file.c to avoid signaling update failure. - Drop retry loop as post-update is best-effort and non-fatal. core/install_from_file.c | 4 +++- tools/swupdate-client.c | 1 - 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/core/install_from_file.c b/core/install_from_file.c index e222e30e..f086f689 100644 --- a/core/install_from_file.c +++ b/core/install_from_file.c @@ -60,7 +60,9 @@ static int endupdate(RECOVERY_STATUS status) ipc_message msg; msg.data.procmsg.len = 0; if (ipc_postupdate(&msg) != 0 || msg.type != ACK) { - end_status = EXIT_FAILURE; + WARN("Post-update actions failed%s%s", + (msg.type != ACK && msg.data.msg[0]) ? ": " : "", + (msg.type != ACK && msg.data.msg[0]) ? msg.data.msg : ""); } } diff --git a/tools/swupdate-client.c b/tools/swupdate-client.c index 5075bfed..3d4ffcfd 100644 --- a/tools/swupdate-client.c +++ b/tools/swupdate-client.c @@ -116,7 +116,6 @@ static int end(RECOVERY_STATUS status) msg.data.procmsg.len = 0; if (ipc_postupdate(&msg) != 0 || msg.type != ACK) { fprintf(stderr, "Running post-update failed!\n"); - end_status = EXIT_FAILURE; } }
Hi Arturs, On 9/22/26 16:46, 'Arturs Laizans' via swupdate wrote: > Hi Stefano, > > Thank you for the quick and clear feedback! > > > Do you experience the same issue if you use "swupdate-client > <filename>" instead of "swupdate -i" ? > > In our embedded setup, SWUpdate is invoked by an updater daemon on-demand > to install local/nested SWU packages on subcomponents, so running > standalone "swupdate -i" was used rather than having a persistent background > daemon running. Looking at tools/swupdate-client.c, line 119 also has: > end_status = EXIT_FAILURE; > so swupdate-client exhibits the exact same behavior if post-update actions > fail or race. > > Your architectural rationale makes total sense: the update itself is > atomic and complete once "SWUpdate was successful !" is reached, and > post-update > actions are best-effort post-processing that should not invalidate the > update. > > In v2: > - Both core/install_from_file.c and tools/swupdate-client.c are updated > so that end_status is preserved as EXIT_SUCCESS if the update succeeded. Ok > - Used WARN() instead of ERROR() in install_from_file.c so it does not > indicate that the update failed. Agree > - Dropped the retry loop since post-update failures are no longer fatal. > Ok > (Note: we noticed corelib/downloader.c:84 also sets result = FAILURE on > ipc_postupdate failure; please let me know if you would like that aligned > in this patch series as well.) Yes, please do it. Thanks, Stefano Babic > > Best regards, > Arturs > > --- > > From 9cb78d5e1af9fd0c353e8241c4ce0181978d76da Mon Sep 17 00:00:00 2001 > From: Arturs Laizans <arturs.laizans.ext@kiongroup.com> > Date: Tue, 22 Sep 2026 16:36:26 +0200 > Subject: [PATCH v2] install_from_file, swupdate-client: Do not fail > update on > post-update failure > > The update itself is atomic and complete once "SWUpdate was successful !" > is reached. Post-update actions initiated via ipc_postupdate() are intended > for subsequent actions or cleanup, but failure of post-update actions > should not invalidate the already successful update or alter its final > exit status. > > Previously, both install_from_file() and swupdate-client's end() > callback overrode end_status to EXIT_FAILURE if ipc_postupdate() failed > or returned NACK. In install_from_file(), this occurred silently without > any error or warning output. > > Do not alter end_status on post-update failures in either > install_from_file() or swupdate-client. Additionally, log a warning via > WARN() in install_from_file() if post-update actions fail, so that > issues with post-processing are visible without marking the update > as failed. > > Signed-off-by: Arturs Laizans <arturs.laizans.ext@kiongroup.com> > --- > Changes in v2: > - Do not override end_status to EXIT_FAILURE on post-update failures > (update was already successful). > - Apply the fix consistently across both core/install_from_file.c and > tools/swupdate-client.c. > - Use WARN() instead of ERROR() in install_from_file.c to avoid > signaling update failure. > - Drop retry loop as post-update is best-effort and non-fatal. > > core/install_from_file.c | 4 +++- > tools/swupdate-client.c | 1 - > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/core/install_from_file.c b/core/install_from_file.c > index e222e30e..f086f689 100644 > --- a/core/install_from_file.c > +++ b/core/install_from_file.c > @@ -60,7 +60,9 @@ static int endupdate(RECOVERY_STATUS status) > ipc_message msg; > msg.data.procmsg.len = 0; > if (ipc_postupdate(&msg) != 0 || msg.type != ACK) { > -end_status = EXIT_FAILURE; > +WARN("Post-update actions failed%s%s", > + (msg.type != ACK && msg.data.msg[0]) ? ": " : "", > + (msg.type != ACK && msg.data.msg[0]) ? msg.data.msg : ""); > } > } > > diff --git a/tools/swupdate-client.c b/tools/swupdate-client.c > index 5075bfed..3d4ffcfd 100644 > --- a/tools/swupdate-client.c > +++ b/tools/swupdate-client.c > @@ -116,7 +116,6 @@ static int end(RECOVERY_STATUS status) > msg.data.procmsg.len = 0; > if (ipc_postupdate(&msg) != 0 || msg.type != ACK) { > fprintf(stderr, "Running post-update failed!\n"); > -end_status = EXIT_FAILURE; > } > } > > -- > 2.43.0 > > On Tuesday, September 22, 2026 at 4:03:05 PM UTC+2 Stefano Babic wrote: > > Hi Arturs, > > On 9/22/26 15:08, 'Arturs Laizans' via swupdate wrote: > > From 77584022e56876c476c318f1dcb14fc3c43bba3c Mon Sep 17 00:00:00 > 2001 > > From: Arturs Laizans <arturs.la...@kiongroup.com> > > Date: Tue, 22 Sep 2026 14:51:53 +0200 > > Subject: [PATCH] install_from_file: Fix silent failure and add > retries for > > post-update IPC > > > > When install_from_file() completes an installation successfully, > > endupdate() issues an ipc_postupdate() call to execute any post- > update > > actions. However, if ipc_postupdate() fails (due to a transient > socket > > connection error, busy IPC listener, or context-switch delay) or if > > the server responds with NACK, endupdate() silently overrides > end_status > > to EXIT_FAILURE without logging any error message. > > Let's start with the intention: install_from_file() is a backward > compatible function, but it is not anymore how SWUpdate is designed to > get the SWU. SWUpdate accepts streams, and in case of local files, they > should be sent via own application or swupdate-client utility. > > The ipc_postupdate() function is thought to start something *AFTER* the > update - this is wanted for cases where users require to clean up > something, add some post processing, but it does not change the status > of the update. The atomicity of the update is guaranteed until > "SWUpdate > was successful !", after that post update scripts or whatever can fail > and they are not tracked. > > So first: same logic and code is implemented in core/ > install_from_file.c > and in tools/swupdate-client.c. Both should be touched because it is > the > same use case. > > I could also remove at all the "-i" option in future because this is > not > part of the design I did, but it was set at the beginning for easy > testing. For design, swupdate is a daemon waiting for incoming streams. > > > > > As a result, users and frontends see: > > [INFO ] : SWUPDATE running : [install_from_file] : SWUpdate was > > successful ! > > immediately followed by the process exiting with status 1 and no > > indication of why the update was considered failed. > > I agree that status of the update shouldn't be changed. > > > > > Furthermore, unlike the start of install_from_file() which retries > > swupdate_async_start() up to 3 times with a 1-second delay to handle > > IPC listener readiness, endupdate() previously attempted > ipc_postupdate() > > Understood, this is a sort of race because the daemon is not really > started, and the client is part of the daemon itself, as said, just for > compatibility reason. But maybe it make smore sense to remove this code > at all. > > > only once without retries, making it susceptible to transient > socket races > > under high load. > > I do not think there is high load, I guess there is a race because > SWUpdate has finished and it is exiting, but in the same time some more > work is issued (ipc_ostupdate). Do you experience the same issue if you > use "swupdate-client <filename>" instead of "swupdate -i" ? > > > > > Add retry logic (up to 3 attempts with 1-second backoff) for > transient > > IPC communication errors in endupdate(), and explicitly report > errors > > via ERROR() when post-update actions or IPC fail. > > ERROR is sent in cases where the Update itself must be considered > failed. The update was successful, a post update something couldn't be > executed with success, but this does not change the status of the > update > itself. > > Best regards, > Stefano Babic > > > > > > Signed-off-by: Arturs Laizans <arturs.la...@kiongroup.com> > > --- > > core/install_from_file.c | 22 ++++++++++++++++++++-- > > 1 file changed, 20 insertions(+), 2 deletions(-) > > > > diff --git a/core/install_from_file.c b/core/install_from_file.c > > index e222e30e..719f613e 100644 > > --- a/core/install_from_file.c > > +++ b/core/install_from_file.c > > @@ -58,8 +58,26 @@ static int endupdate(RECOVERY_STATUS status) > > > > if (status == SUCCESS) { > > ipc_message msg; > > -msg.data.procmsg.len = 0; > > -if (ipc_postupdate(&msg) != 0 || msg.type != ACK) { > > +int ret = -1; > > +int retries = 3; > > + > > +while (retries > 0) { > > +memset(&msg, 0, sizeof(msg)); > > +msg.data.procmsg.len = 0; > > +ret = ipc_postupdate(&msg); > > +if (ret == 0) > > +break; > > +retries--; > > +if (retries > 0) > > +sleep(1); > > +} > > + > > +if (ret != 0) { > > +ERROR("Post-update actions failed: IPC communication error"); > > +end_status = EXIT_FAILURE; > > +} else if (msg.type != ACK) { > > +ERROR("Post-update actions failed: %s", > > + msg.data.msg[0] ? msg.data.msg : "rejected by server"); > > end_status = EXIT_FAILURE; > > } > > } > > -- > > 2.43.0 > > > > -- > > You received this message because you are subscribed to the Google > > Groups "swupdate" group. > > To unsubscribe from this group and stop receiving emails from it, > send > > an email to swupdate+u...@googlegroups.com > > <mailto:swupdate+u...@googlegroups.com>. > > To view this discussion visit https://groups.google.com/d/msgid/ > <https://groups.google.com/d/msgid/> > > swupdate/72ec038f-df3f-4ad2-9c73-8241eab4b97en%40googlegroups.com > <http://40googlegroups.com> > > <https://groups.google.com/d/msgid/swupdate/72ec038f- <https:// > groups.google.com/d/msgid/swupdate/72ec038f-> > > df3f-4ad2-9c73-8241eab4b97en%40googlegroups.com > <http://40googlegroups.com>? > > utm_medium=email&utm_source=footer>. > > -- > You received this message because you are subscribed to the Google > Groups "swupdate" group. > To unsubscribe from this group and stop receiving emails from it, send > an email to swupdate+unsubscribe@googlegroups.com > <mailto:swupdate+unsubscribe@googlegroups.com>. > To view this discussion visit https://groups.google.com/d/msgid/ > swupdate/bb5118c7-3d27-482c-b1b7-813045572799n%40googlegroups.com > <https://groups.google.com/d/msgid/swupdate/bb5118c7-3d27-482c- > b1b7-813045572799n%40googlegroups.com?utm_medium=email&utm_source=footer>.
Hi Stefano, Thanks! Here is v3 aligning corelib/downloader.c as well: --- From 21bd54386e6ae93b55b843a08a287bcdd8db68d2 Mon Sep 17 00:00:00 2001 From: Arturs Laizans <arturs.laizans.ext@kiongroup.com> Date: Tue, 22 Sep 2026 16:36:26 +0200 Subject: [PATCH v3] install_from_file, downloader, swupdate-client: Do not fail update on post-update failure The update itself is atomic and complete once "SWUpdate was successful !" is reached. Post-update actions initiated via ipc_postupdate() are intended for subsequent actions or cleanup, but failure of post-update actions should not invalidate the already successful update or alter its final exit status. Previously, install_from_file(), download_from_url(), and swupdate-client's end() callback all overrode the success status to failure if ipc_postupdate() failed or returned NACK. In install_from_file() and download_from_url(), this occurred silently without any error or warning output. Do not alter the update status on post-update failures in install_from_file(), downloader, or swupdate-client. Additionally, log a warning via WARN() in install_from_file() and downloader if post-update actions fail, so that issues with post-processing are visible without marking the update as failed. Signed-off-by: Arturs Laizans <arturs.laizans.ext@kiongroup.com> --- Changes in v3: - Align corelib/downloader.c: do not set result to FAILURE on post-update failure, log warning via WARN(). Changes in v2: - Do not override end_status to EXIT_FAILURE on post-update failures (update was already successful). - Apply the fix consistently across both core/install_from_file.c and tools/swupdate-client.c. - Use WARN() instead of ERROR() in install_from_file.c to avoid signaling update failure. - Drop retry loop as post-update is best-effort and non-fatal. core/install_from_file.c | 4 +++- corelib/downloader.c | 4 +++- tools/swupdate-client.c | 1 - 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/core/install_from_file.c b/core/install_from_file.c index e222e30e..f086f689 100644 --- a/core/install_from_file.c +++ b/core/install_from_file.c @@ -60,7 +60,9 @@ static int endupdate(RECOVERY_STATUS status) ipc_message msg; msg.data.procmsg.len = 0; if (ipc_postupdate(&msg) != 0 || msg.type != ACK) { - end_status = EXIT_FAILURE; + WARN("Post-update actions failed%s%s", + (msg.type != ACK && msg.data.msg[0]) ? ": " : "", + (msg.type != ACK && msg.data.msg[0]) ? msg.data.msg : ""); } } diff --git a/corelib/downloader.c b/corelib/downloader.c index 81015ca2..71908d44 100644 --- a/corelib/downloader.c +++ b/corelib/downloader.c @@ -83,7 +83,9 @@ static RECOVERY_STATUS download_from_url(channel_data_t* channel_data) ipc_message msg; msg.data.procmsg.len = 0; if (ipc_postupdate(&msg) != 0 || msg.type != ACK) { - result = FAILURE; + WARN("Post-update actions failed%s%s", + (msg.type != ACK && msg.data.msg[0]) ? ": " : "", + (msg.type != ACK && msg.data.msg[0]) ? msg.data.msg : ""); } } diff --git a/tools/swupdate-client.c b/tools/swupdate-client.c index 5075bfed..3d4ffcfd 100644 --- a/tools/swupdate-client.c +++ b/tools/swupdate-client.c @@ -116,7 +116,6 @@ static int end(RECOVERY_STATUS status) msg.data.procmsg.len = 0; if (ipc_postupdate(&msg) != 0 || msg.type != ACK) { fprintf(stderr, "Running post-update failed!\n"); - end_status = EXIT_FAILURE; } }
diff --git a/core/install_from_file.c b/core/install_from_file.c index e222e30e..719f613e 100644 --- a/core/install_from_file.c +++ b/core/install_from_file.c @@ -58,8 +58,26 @@ static int endupdate(RECOVERY_STATUS status) if (status == SUCCESS) { ipc_message msg; - msg.data.procmsg.len = 0; - if (ipc_postupdate(&msg) != 0 || msg.type != ACK) { + int ret = -1; + int retries = 3; + + while (retries > 0) { + memset(&msg, 0, sizeof(msg)); + msg.data.procmsg.len = 0; + ret = ipc_postupdate(&msg); + if (ret == 0) + break; + retries--; + if (retries > 0) + sleep(1); + } + + if (ret != 0) { + ERROR("Post-update actions failed: IPC communication error"); + end_status = EXIT_FAILURE; + } else if (msg.type != ACK) { + ERROR("Post-update actions failed: %s", + msg.data.msg[0] ? msg.data.msg : "rejected by server"); end_status = EXIT_FAILURE; } }