fix: return UpdateInProgress status instead of erroring when another update is running (#335)
When `update()` is called while another update (typically the automatic updater thread) is already running, the Rust updater previously bailed with `UpdateError::UpdateAlreadyInProgress`, which surfaced in Dart as `UpdateException: Update already in progress (unknown)`. This is the single highest-volume `UpdateException` in customer telemetry, yet the underlying situation is benign — the in-flight update continues on its own, the caller simply did not start a new one. Add a new `UpdateStatus::UpdateInProgress` variant and matching C status code `SHOREBIRD_UPDATE_IN_PROGRESS = 4`. `updater::update()` catches the `UpdateAlreadyInProgress` error from the lock helper and maps it to `Ok(UpdateStatus::UpdateInProgress)`. The Dart wrapper treats the new status as a successful return alongside `SHOREBIRD_UPDATE_INSTALLED`, so `update()` no longer throws for this case. Update the existing `usage_during_hung_update` c_api test to assert the new contract, and add a Dart test covering the in-progress return path. Version skew: new Dart on an old engine still sees the legacy `SHOREBIRD_UPDATE_ERROR` + "Update already in progress" message and will still throw. The fix lands once both sides ship. Partially addresses shorebirdtech/shorebird#3682 — does not resolve the broader asymmetry of `update()` semantics (it still does not wait for someone else's in-flight update to finish), which remains as v2 design work in shorebirdtech/shorebird#3684.
This commit is contained in:
@@ -41,6 +41,13 @@
|
||||
*/
|
||||
#define SHOREBIRD_UPDATE_IS_BAD_PATCH 3
|
||||
|
||||
/**
|
||||
* Another update was already in progress when this call was made. The
|
||||
* already-running update will continue; the caller did not start a new one.
|
||||
* This is a benign outcome, not an error.
|
||||
*/
|
||||
#define SHOREBIRD_UPDATE_IN_PROGRESS 4
|
||||
|
||||
/**
|
||||
* Struct containing configuration parameters for the updater.
|
||||
* Passed to all updater functions.
|
||||
|
||||
@@ -63,6 +63,11 @@ pub const SHOREBIRD_UPDATE_HAD_ERROR: i32 = 2;
|
||||
/// The downloaded patch was not installed because it was invalid.
|
||||
pub const SHOREBIRD_UPDATE_IS_BAD_PATCH: i32 = 3;
|
||||
|
||||
/// Another update was already in progress when this call was made. The
|
||||
/// already-running update will continue; the caller did not start a new one.
|
||||
/// This is a benign outcome, not an error.
|
||||
pub const SHOREBIRD_UPDATE_IN_PROGRESS: i32 = 4;
|
||||
|
||||
#[repr(C)]
|
||||
pub struct UpdateResult {
|
||||
pub status: i32,
|
||||
@@ -1007,7 +1012,13 @@ mod test {
|
||||
shorebird_start_update_thread();
|
||||
// Wait for the thread to start.
|
||||
std::thread::sleep(std::time::Duration::from_millis(100));
|
||||
assert!(updater::update(None).is_err());
|
||||
// When another update is already in progress, `update()` returns
|
||||
// `UpdateStatus::UpdateInProgress` rather than surfacing an error.
|
||||
// The in-flight update continues on its own.
|
||||
assert_eq!(
|
||||
updater::update(None).unwrap(),
|
||||
crate::UpdateStatus::UpdateInProgress
|
||||
);
|
||||
}
|
||||
// Unlock the lock, and wait for the thread to finish.
|
||||
std::thread::sleep(std::time::Duration::from_millis(100));
|
||||
|
||||
+22
-1
@@ -30,6 +30,10 @@ pub enum UpdateStatus {
|
||||
UpdateInstalled,
|
||||
UpdateHadError,
|
||||
UpdateIsBadPatch,
|
||||
// Another update was already in progress when this call was made. The
|
||||
// already-running update will continue; the caller did not start a new
|
||||
// one. This is a benign outcome, not an error.
|
||||
UpdateInProgress,
|
||||
}
|
||||
|
||||
impl Display for UpdateStatus {
|
||||
@@ -42,6 +46,7 @@ impl Display for UpdateStatus {
|
||||
f,
|
||||
"Update available but previously failed to install. Not installing."
|
||||
),
|
||||
UpdateStatus::UpdateInProgress => write!(f, "Update already in progress"),
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -651,7 +656,23 @@ fn cleanup_download_artifacts(download_path: &Path) {
|
||||
|
||||
/// Synchronously checks for an update and downloads and installs it if available.
|
||||
pub fn update(channel: Option<&str>) -> anyhow::Result<UpdateStatus> {
|
||||
with_updater_thread_lock(|lock_state| update_internal(lock_state, channel))
|
||||
match with_updater_thread_lock(|lock_state| update_internal(lock_state, channel)) {
|
||||
Ok(status) => Ok(status),
|
||||
Err(e) => {
|
||||
// "Another update is already running" is a benign outcome — the
|
||||
// in-progress update (typically the automatic updater thread) will
|
||||
// continue on its own. Surface it as a non-error status so callers
|
||||
// that monitor `update()` exceptions do not see it as a failure.
|
||||
if matches!(
|
||||
e.downcast_ref::<UpdateError>(),
|
||||
Some(UpdateError::UpdateAlreadyInProgress)
|
||||
) {
|
||||
Ok(UpdateStatus::UpdateInProgress)
|
||||
} else {
|
||||
Err(e)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// The first 4 bytes of any zstd compressed frame.
|
||||
|
||||
@@ -5116,3 +5116,5 @@ const int SHOREBIRD_UPDATE_INSTALLED = 1;
|
||||
const int SHOREBIRD_UPDATE_HAD_ERROR = 2;
|
||||
|
||||
const int SHOREBIRD_UPDATE_IS_BAD_PATCH = 3;
|
||||
|
||||
const int SHOREBIRD_UPDATE_IN_PROGRESS = 4;
|
||||
|
||||
@@ -117,12 +117,15 @@ class ShorebirdUpdaterImpl implements ShorebirdUpdater {
|
||||
|
||||
final status = result.ref.status;
|
||||
|
||||
// SHOREBIRD_UPDATE_INSTALLED is the success case. SHOREBIRD_NO_UPDATE
|
||||
// (the app is already up to date) is also a successful outcome of a call
|
||||
// to update() and must not throw — previously it surfaced as a confusing
|
||||
// `UpdateException: No update (noUpdate)` in customer telemetry.
|
||||
// Successful outcomes of update():
|
||||
// - SHOREBIRD_UPDATE_INSTALLED: a new patch was downloaded and installed.
|
||||
// - SHOREBIRD_NO_UPDATE: the app is already running the latest patch.
|
||||
// - SHOREBIRD_UPDATE_IN_PROGRESS: another update (typically the automatic
|
||||
// updater thread) was already running; the caller did not start a new
|
||||
// one. This is benign and must not surface as an exception.
|
||||
if (status == SHOREBIRD_UPDATE_INSTALLED ||
|
||||
status == SHOREBIRD_NO_UPDATE) {
|
||||
status == SHOREBIRD_NO_UPDATE ||
|
||||
status == SHOREBIRD_UPDATE_IN_PROGRESS) {
|
||||
return;
|
||||
}
|
||||
|
||||
|
||||
@@ -411,6 +411,33 @@ void main() {
|
||||
});
|
||||
});
|
||||
|
||||
group('when another update is already in progress', () {
|
||||
setUp(() {
|
||||
when(() => updater.currentPatchNumber()).thenReturn(0);
|
||||
final result = calloc.allocate<UpdateResult>(sizeOf<UpdateResult>());
|
||||
result.ref.status = SHOREBIRD_UPDATE_IN_PROGRESS;
|
||||
result.ref.message =
|
||||
'Update already in progress'.toNativeUtf8().cast<Char>();
|
||||
addTearDown(() {
|
||||
calloc
|
||||
..free(result.ref.message)
|
||||
..free(result);
|
||||
});
|
||||
when(() => updater.update()).thenReturn(result);
|
||||
shorebirdUpdater = ShorebirdUpdaterImpl(updater: updater, run: run);
|
||||
});
|
||||
|
||||
test('returns normally and does not throw', () async {
|
||||
// When the Rust updater reports that another update is already
|
||||
// running, `update()` must not surface it as an exception — it is
|
||||
// a benign outcome. The already-running update continues on its
|
||||
// own; the caller simply did not start a new one.
|
||||
await expectLater(shorebirdUpdater.update(), completes);
|
||||
verify(updater.update).called(1);
|
||||
verify(() => updater.freeUpdateResult(any())).called(1);
|
||||
});
|
||||
});
|
||||
|
||||
group('when an error occurs during download', () {
|
||||
setUp(() {
|
||||
when(() => updater.currentPatchNumber()).thenReturn(0);
|
||||
|
||||
Reference in New Issue
Block a user