Lands the fake HTTP server, the patch fixture pipeline, and the first
two scenarios that exercise the real FFI path end-to-end. Plus a test
that would have caught the patch-to-release rollback bug
(shorebirdtech/shorebird#3728).
Architecture follows the principles surfaced in review:
- Dart tests call the public `ShorebirdUpdater` API only — never the
raw `Updater` FFI wrapper, never the engine API.
- Engine API stays inside the `library_test_hooks` Rust crate.
`shorebird_test_init` constructs `AppParameters` + stub
`FileCallbacks` internally so Dart never sees those types.
`shorebird_test_simulate_successful_launch` wraps the
start/success protocol so the Dart layer never knows there's a
protocol — it just knows "the engine reported a successful boot."
- ffigen scans only the test_hooks header. The engine header
(updater_engine.h) does not appear in the Dart bindings.
- A `TestEngine` Dart helper concentrates engine-side simulation in
one place; the test bodies stay focused on `ShorebirdUpdater`.
Implementation choices worth flagging:
- FFI calls run via `Isolate.run`. Synchronous run blocks the main
isolate, deadlocking against the in-isolate shelf server. The
test's IsolateRun callback re-opens the cdylib (cheap: dlopen is
ref-counted) and resets `Updater.bindings` in the sub-isolate
because Dart isolates do not share static fields.
- `libapp_path` must be a real file on the desktop integration build:
the non-Android non-iOS non-test `patch_base` reads it directly
from disk. Tests that install a patch write the fixture's `base`
bytes to `libapp.so` before init.
- Fake server kept minimal: shelf, no Range support, no auth, no
concurrency knobs. Stage 3+ scenarios (download cutoff, hash
mismatch loop, etc.) extend it as needed.
Three scenarios cover three reasons we wanted this suite:
1. `checkForUpdate returns upToDate when server has no patch` —
baseline: confirms the harness boots cleanly and returns the
expected enum.
2. `install a patch and boot from it` — golden path: check →
update → simulateSuccessfulLaunch, then assert
`readCurrentPatch` / `readNextPatch` / `checkForUpdate`
transitions match the public API contract.
3. `checkForUpdate returns restartRequired after patch-to-release
rollback` — regression for shorebirdtech/shorebird#3728. Pre-fix
this returned `upToDate` and left no signal to prompt a restart.
Verified locally: 232 Rust unit tests + 44 Dart tests (41 existing
unit + 3 new integration) green; clippy/fmt/cspell clean.
* perf: mmap libapp.so out of the APK instead of buffering in RAM
`open_base_lib` previously read the entire decompressed libapp.so into a
Vec<u8> via `read_to_end` and handed bipatch a Cursor over that buffer.
For large apps libapp.so can be tens of megabytes, and that allocation
happens immediately after the patch download is buffered to disk — a
plausible OOM trigger on memory-constrained devices (we have at least
one customer report on OnePlus where the patch install appears to halt
silently right after the download completes).
Modern AGP (3.6+) defaults to extractNativeLibs=false, which keeps
libapp.so STORED uncompressed inside the APK so the dynamic linker can
mmap it directly. When that's the case, do the same: find the entry's
data offset via the zip crate, drop the archive, reopen the APK, and
mmap the entry's slice. Cursor<Mmap> implements Read + Seek, which is
what bipatch's `Reader::new(patch, base)` requires.
When the entry isn't stored uncompressed (older builds, or builds that
explicitly compress native libs), fall back to the previous buffered
read so we always succeed.
Mmap doesn't change the peak working set when bipatch traverses the
whole base linearly, but file-backed mappings are clean and reclaimable
under memory pressure where an anonymous Vec is not, and we avoid the
~2x transient allocation peak from `read_to_end` growing the buffer.
Tests cover both paths (stored → mmap, deflated → buffered) on the host.
* ci: add mmap, memmap, SIGBUS to cspell dictionary
* docs: integration tests design proposal
Adds docs/integration_tests.md proposing a desktop-only integration suite
that drives the Dart `ShorebirdUpdater` API against the real Rust core via
FFI, with a fake HTTP server and per-test tempdir state. Describes a new
`library_test_hooks` cdylib that reaches into `updater` via a `test-hooks`
Cargo feature, so production C API stays clean. Status: design exploration,
not a committed plan.
* feat: stage 1 integration test harness for shorebird_code_push
Lands the test_hooks crate, the Dart-side loader, and one trivial
end-to-end test, per docs/integration_tests.md stage 1.
Pieces:
- New `library_test_hooks` workspace crate (cdylib). Depends on `updater`
with a new `test-hooks` Cargo feature that widens the visibility of
internal items (currently `testing_reset_config`) for sibling-crate
access only — production builds do not enable it. The crate also
re-exports `updater::c_api::dart::*` and `updater::c_api::engine::*`,
which keeps the rlib's `#[no_mangle]` symbols out of DCE so the
resulting cdylib carries both the production C API and the new
`shorebird_test_*` hooks.
- Workspace `default-members` excludes `library_test_hooks` so plain
`cargo build` / `cargo test` invocations don't unify the `test-hooks`
feature into production builds.
- `shorebird_code_push/test/integration/all_test.dart` (single file,
with a header comment explaining why) loads the cdylib via
`DynamicLibrary.open`, reassigns the existing `@visibleForTesting`
`Updater.bindings` setter, and exercises both surfaces. The build
helper shells out to `cargo build -p library_test_hooks`; if it
fails, `markTestSkipped` keeps the suite green on machines without
a working Rust toolchain.
- ffigen config (`ffigen_test_hooks.yaml`) generates the test-only
Dart bindings under `test/integration/generated/`, not `lib/`.
- CI: `library_test_hooks` is added to the rust_crate matrix, and the
shorebird_code_push job triggers on `library/**` and
`library_test_hooks/**` so cdylib changes can't break the Dart-side
integration suite without CI noticing.
Verified locally: 232 Rust unit tests + 42 Dart tests (41 existing + 1
integration) green; clippy/fmt/cspell clean; production cdylib does
not contain `shorebird_test_reset` (`nm` confirms feature isolation).
Stages 2 (FakePatchServer + golden path) and 3 (adversarial scenarios)
land separately.
* fix(integration test): early-return on skip and bump per-test timeout
Two issues caught by Shorebird CI on PR #353:
1. `markTestSkipped` does not abort test execution — it only flags the
test as skipped on its way out. The body kept running and crashed on
the `late testHooks` field when the cdylib build had failed in
setUpAll. Move the skip check into the test body itself with an
early return; drop the (no-op) skip in `setUp`.
2. Default per-test timeout (30s) also covers `setUpAll`. A cold
`cargo build -p library_test_hooks` compiles `updater` and ~100
transitive deps, which can run minutes on CI. Bump to 10 minutes
via `@Timeout` on the library.
Verified locally: passes when cargo is on PATH (1 passed), reports a
clean skip and exit 0 when cargo is removed from PATH (1 skipped).
* refactor(integration test): drop ! by promoting testHooks to late final
`testHooks` was nullable so accessing it after the skipReason check
required `!`, and `markTestSkipped(skipReason!)` had the same smell.
Make `testHooks` `late final` (non-nullable, throws if read before
setUpAll assigns) and pull `skipReason` into a non-null local before
use. Same control flow, no bang operators.
* feat: send current patch_number on patch check
* refactor: rename patch_number → current_patch_number
Renaming the field on PatchCheckRequest so it doesn't collide with the
legacy `patch_number` field old pre-#189 updaters still send. That
legacy field is what triggers the server's short-circuit response
(patchAvailable: false, no rolled_back_patch_numbers), which is still
load-bearing for ~0.7% of patch-check traffic coming from Flutter
≤ 3.22.2 clients. Using a distinct field name keeps our new analytics
signal from accidentally engaging that path.
Pairs with:
- shorebirdtech/shorebird#3702 (protocol field)
- shorebirdtech/_shorebird#2059 (server consumes this field)
* docs: rephrase current_patch_number doc comment
Drops the 'analytics' framing in favor of describing the field as
superseding patch_number for newer clients. patch_number remains for
compatibility with legacy clients that rely on the short-circuit path.
* fix: adapt to renamed currently_booting_patch and &str client_id
The C surface in `library/src/c_api` was a single bucket of `pub extern "C"`
functions covering both consumers — `package:shorebird_code_push` (via
ffigen) and Shorebird's Flutter engine fork (via direct C++ link). That
made it hard to reason about which symbols are stable ABI versus
internal, and ffigen was generating bindings for engine-only symbols
that no Dart code calls.
Split into two self-contained submodules and two cbindgen-generated
headers:
- `c_api::dart` → `include/updater_dart.h` (stable ABI; ffigen entry
point). Defines `UpdateResult`, the `SHOREBIRD_*` status constants, and
the five Dart-stable functions: `shorebird_current_boot_patch_number`,
`shorebird_next_boot_patch_number`,
`shorebird_check_for_downloadable_update`,
`shorebird_update_with_result`, `shorebird_free_update_result`.
- `c_api::engine` → `include/updater_engine.h` (no stability guarantee).
Defines `AppParameters`, `FileCallbacks`, and the engine-only functions:
`shorebird_init`, `shorebird_should_auto_update`,
`shorebird_validate_next_boot_patch`, `shorebird_next_boot_patch_path`,
`shorebird_free_string`, `shorebird_start_update_thread`, and the
`shorebird_report_launch_*` trio.
Each bucket file is self-contained: cbindgen scans only the file
(`with_src` in build.rs) and emits the items it defines plus the C
types they reference. There are no exclude/include lists in the
cbindgen configs — adding a function to one bucket automatically lands
it in the right header, and items in the other bucket cannot leak.
`mod.rs` shrinks to a thin layer of private helpers shared by both
buckets (`to_rust`, `allocate_c_string`, `free_c_string`, `log_on_error`)
plus the test module.
`include/updater.h` is removed; consumers include the specific header
for their use case. The Flutter engine's
`shell/common/shorebird/updater.cc` will be updated in a follow-up
engine-repo PR to include `updater_engine.h` directly.
Also drops two retired Dart-side symbols:
- `shorebird_update` (replaced by `shorebird_update_with_result` in the
Dart 2.0 rewrite, Nov 2024).
- `shorebird_check_for_update` (replaced by
`shorebird_check_for_downloadable_update` in the same rewrite).
The shorebird_code_push package's `_legacyFallback` was the only path
that still called `shorebird_update`. The package's `flutter: >=3.24.5`
constraint guarantees the engine has `shorebird_update_with_result`, so
the fallback was unreachable in practice. Removing it lets us drop the
ABI symbol.
Bumps shorebird_code_push to 2.0.7. Bindings regenerated via ffigen now
contain only the five Dart-stable symbols.
Follow-up engine PR will: include `updater_engine.h` instead of the
removed `updater.h`; clean up `android_exports.lst` (drop the ghost
`shorebird_active_path` and `shorebird_active_patch_number` exports,
drop `shorebird_check_for_update`).
* fix: skip fetch when prior download is already complete on disk
A prior attempt that finished downloading but failed a post-download
step (inflate / hash check / install) used to leave the partial file
and sidecar in place. The next update would call compute_resume_offset,
get back the file's full size, and send Range: bytes=N- against an
N-byte resource — past the end of the file, yielding HTTP 416 forever.
Replace compute_resume_offset with a read-only
determine_download_start_state returning Fresh | Resume(u64) |
Complete(u64). update_internal handles Complete by skipping
download_to_path entirely and letting the install path validate the
existing bytes; this also avoids re-downloading when the app is killed
between download and install.
Extract the post-download work into install_downloaded_patch so cleanup
can run unconditionally once after it returns. Three previous cleanup
sites collapse to one. If install rejects the bytes, the next attempt
re-downloads from scratch.
* refactor: pass output_path by value into install_downloaded_patch
Avoids a redundant PathBuf clone on the success path. The caller already
owns the PathBuf from download_dir.join(...), and PatchInfo wants an
owned path, so threading ownership through is strictly cheaper than
borrowing and cloning.
* fix: record actual download size in sidecar to handle chunked transfers
When the server uses chunked transfer encoding (no Content-Length header),
dl_result.content_length is None. The old code recorded that None as
expected_size, which meant a subsequent crash-before-install attempt
would see expected_size: None + a full-size file, fall through to
Resume(file_size), and re-create the HTTP 416 loop this PR fixes.
Recording dl_result.total_bytes (the actual on-disk size) closes the
chunked-encoding case. The two values are equal when Content-Length is
present, so the Content-Length case is unchanged.
Also annotate two pre-existing windows that this PR doesn't fully close,
so they stay visible until the per-patch state machine refactor lands:
- The microsecond gap between download_to_path returning and the second
write_download_state succeeding can still leave expected_size: None.
- cleanup_download_artifacts logs delete errors instead of propagating;
a silently-failed delete could re-create the same 416 loop.
Both are tracked in shorebirdtech/shorebird#3737.
* fix: current_boot_patch survives server-driven rollback
Customer report (shorebirdtech/shorebird#3728): when the device's running
patch is rolled back to the base release (no replacement patch), the
running session sees `checkForUpdate` return `upToDate` even though a
restart is needed. Patch-to-patch rollback works because the server's
replacement patch makes `check_for_downloadable_update` return true, so
Dart short-circuits to `outdated` before the comparison runs.
Root cause: `UpdaterState::current_boot_patch()` derived its return value
from `currently_booting_patch.or(last_successfully_booted_patch)`. After
boot success, only `last_booted_patch` reflected the running patch. When
the server rolled back that patch, `try_fall_back_from_patch` cleared
`last_booted_patch` (correctly — it's no longer a valid fallback), and
the FFI `shorebird_current_boot_patch_number` then reported 0 even
though the process was still running the rolled-back patch.
The conflation: `last_booted_patch` was doing two unrelated jobs —
"fallback target" (its real role) and "what's running" (the proxy via
`.or()` that broke under rollback). The earlier Dart-only fix in
shorebirdtech/updater#312 assumed the FFI would still report the
running patch number; that assumption only held in the mock.
Fix: introduce a dedicated `current_boot_patch: Option<usize>` field
on `PatchesState`. Set by `report_launch_start` from `next_boot_patch`
(or `None` for a release boot). Read directly by
`UpdaterState::current_boot_patch()` — no derivation, no fallback.
Each field now has exactly one job:
- `last_booted_patch`: fallback target for `try_fall_back_from_patch`.
Doc updated to remove the "(usually the currently running patch)"
parenthetical that perpetuated the conflation.
- `current_boot_patch` (new): what this process is using. Survives
rollbacks of that patch (the process is still using it). Reset on
the next `report_launch_start` — including `None` on a release
boot, so it doesn't go stale.
- `currently_booting_patch`: unchanged. Still the boot-in-progress
flag for crash detection on the next init.
C API surface unchanged. `shorebird_current_boot_patch_number` still
returns the same `usize` it always has — it just gets the right answer
under rollback now.
Verification (testing at the C API level, since that's the contract):
- New regression test `rollback_to_release_keeps_current_boot_patch`
reproduces the customer's bug. Fails on the parent commit
(`current_boot_patch_number` returns 0); passes after this fix
(returns 1).
- New `rollback_to_release_then_restart_clears_current_boot_patch`
proves the post-restart cleanup: the on-disk `current_boot_patch`
is `Some(1)` from the previous run, but the next launch's
`report_launch_start` resets it to `None` since `next_boot_patch`
is `None`. No false-positive `restartRequired` on the release boot.
- New `rollback_patch_to_patch_reports_current_and_next_distinctly`
proves we didn't break the patch-to-patch case. Running on patch 2,
server rolls back to patch 1: after `update()`, `current=2, next=1`.
- All 225 existing tests pass without modification, including every
C API test.
Refs: shorebirdtech/shorebird#3728, shorebirdtech/updater#312, #270
* docs: TODOs for follow-up cleanup of patch state model
Two cleanups deferred from #348 to keep the rollback fix focused:
1. Rename `last_booted_patch` → `fallback_patch`. Single mechanical
rename, but touches ~30 test names that read in terms of the
current field name.
2. Remove `currently_booting_patch` entirely. With `current_boot_patch`
now tracking what's running, the boot-in-progress signal collapses
to `boot_started_at.is_some()`, and the crashed-patch-on-init
identification falls out of the previous run's `current_boot_patch`.
This is the larger of the two — touches crash-detection logic and
the boot-record helpers.
Both should land as their own commits so the diff for each is easy to
read and the rollback fix stays minimal.
* test: assert rollback-only phases never report events
In `rollback_to_release_keeps_current_boot_patch` and
`rollback_to_release_then_restart_clears_current_boot_patch`, the
phase that performs only the server-driven rollback never calls
`shorebird_update` or `shorebird_report_launch_*`, so no event
should ever be reported during it. Replace the no-op report hook
with `UNEXPECTED_REPORT` to make that an asserted property of the
test rather than a silent assumption — if a future change starts
queueing or sending events from `check_for_downloadable_update`,
these tests will surface it immediately.
Phase-1 spawned threads (PatchDownload, PatchInstallSuccess) are
unaffected: they hold a clone of the config from when they were
spawned, so they hit the phase-1 hooks and never reach phase-2's
panicking handler.
The patch-to-patch test keeps the no-op hook because phase 2 there
calls `shorebird_update`, which legitimately spawns a PatchDownload
event using the new hooks.
* docs: flag the last_booted_patch conflation as the underlying bug
Replace the rename TODO with one that names the actual unfixed bug:
`last_booted_patch` gets cleared in `try_fall_back_from_patch` while
the running process is still using the patch. That's the deeper
incoherence — the field's name and `record_boot_success` say it's a
historical record, but the rollback path treats it as an operational
fallback target. Those two roles only diverge under server rollback,
which is the customer's case.
This PR sidesteps the conflation by adding `current_boot_patch` for
the "what's running" semantic. The TODOs now flag both:
- The conflation itself, on the field declaration.
- The specific line in try_fall_back_from_patch that does the
historically-incorrect clearing.
A sibling PR will prototype the alternative — keep last_booted_patch
historical, express "don't fall back to this patch" via a separate
signal — so we can compare the two approaches.
* fix: stop clearing last_booted_patch when its patch is rolled back
Roll #349 into this PR. Both fixes together — they address different
real bugs and combining eliminates each PR's loose ends.
Underlying data-model bug: `last_booted_patch` was conflated. Its
field name and `record_boot_success` say it's a *historical* record
(\"the patch that last successfully booted, ever\"). But
`try_fall_back_from_patch`'s \"both bad\" branch clears it whenever
the patch becomes invalid as a fallback — including when the server
rolls it back, while the running process is still using it.
Fix: in the \"both bad\" branch, only clear `next_boot_patch`. Leave
`last_booted_patch` alone — that history shouldn't change because
the server told us not to use the patch next time. The \"don't fall
back to this patch\" intent is already covered by:
- `delete_patch_artifacts(bad_patch_number)` at the top of the
function, which removes the on-disk artifacts.
- `validate_patch_is_bootable` in the else-if branch, which refuses
to fall back to a patch with missing artifacts.
- `is_known_bad_patch`, which records boot failures explicitly.
`record_boot_failure_for_patch` flows through the same branch and
benefits from the same correction — boot history is preserved across
boot failures. Updated the corresponding test
(`clears_last_booted_patch_if_it_is_the_failed_patch` →
`preserves_last_booted_patch_on_failure_but_marks_bad`) to assert
the new behavior: history preserved, known-bad recorded, artifacts
deleted.
Removes the two TODOs added in the previous commit:
- The conflation TODO on `last_booted_patch` (now fixed).
- The TODO on the offending line (line is being changed).
This pairs with the `current_boot_patch` field added earlier in the
same PR. The two fixes are orthogonal:
- `current_boot_patch` gives us a session-scoped \"what's running\"
signal, reset on `report_launch_start`. It's what the FFI reads.
- The data-model fix here keeps `last_booted_patch` historically
accurate, so the field's name finally matches what it stores.
With both, `current_boot_patch()` no longer needs the `.or()`
fallback that was the original source of the customer's bug.
* feat: enrich patch install failure messages with diagnostics
The `message` field in `__patch_install_failure__` events previously
contained generic strings that didn't help distinguish failure causes.
Crash recovery messages now include:
- `elapsed_secs`: time between boot start and crash recovery detection,
helping distinguish immediate crashes (likely OOM) from delayed kills
(user force-stop, OS reclaim hours later)
- `file_ok`/`file_size`: whether the patch file is intact at recovery
time, catching corruption or partial writes
Message format changes:
- Crash recovery: "crash_recovery: patch N failed to boot
(elapsed_secs=S,file_ok=bool,file_size=N)"
- Engine failure: "engine_report: patch N failed to launch"
Also adds `boot_started_at` timestamp to PatchesState (backward
compatible — older state files deserialize it as None).
* chore: fix formatting
* test: add coverage for crash recovery edge cases
- crash_recovery_with_missing_file: patch artifact deleted before
recovery, verifies file_ok=false,file_missing in message
- crash_recovery_without_boot_timestamp: old state file without
boot_started_at field, verifies elapsed_secs=unknown in message
* refactor: simplify file check in crash recovery diagnostics
Remove unreachable branch: Path::exists() calls fs::metadata()
internally, so if metadata fails, exists() returns false. The
separate file_unreadable case could never be reached.
* refactor: use raw timestamps instead of elapsed_secs in crash recovery
elapsed_secs was misleading because it included the time between the
crash and the user reopening the app — not just the boot duration.
Now reports boot_started_at (raw Unix timestamp) and detected_at (when
crash recovery ran). Server-side analysis can compute elapsed time and
cross-reference with successful boot durations from other devices.
Example: "crash_recovery: patch 1 failed to boot
(detected_at=1776900000,boot_started_at=1776899990,file_ok=true,file_size=524288)"
Reduces binary size of the updater staticlib when linked into the
Flutter engine. On host macOS arm64 the dylib shrinks from 3.07 MB
to 2.56 MB; the biggest iOS savings come from dropping the DWARF
unwind tables (__eh_frame, __gcc_except_tab, __unwind_info) that
panic=unwind generates.
- panic = "abort" in [profile.release]: kills unwind-table generation
and eliminates dead panic-unwinding code paths. We don't use
catch_unwind anywhere in the library. log-panics still runs its
panic hook before abort, so panic messages continue to surface to
logcat/oslog during development.
- Drop `backtrace` feature from anyhow and `with-backtrace` from
log-panics: removes gimli + addr2line + rustc_demangle (~65 KB of
symbolication machinery) that customers can't read anyway. A
future in-engine crash reporter will symbolize native stacks,
making these features redundant.
* fix: surface flush errors and write atomically in disk_io::write
`BufWriter`'s `Drop` impl silently discards flush errors. Because
`patches_state.json` and `state.json` are small enough to fit inside
`BufWriter`'s 8 KB buffer, the only time the bytes actually reach disk
is during the implicit flush at drop — and that flush's I/O errors
(transient iOS Data Protection lock, ENOSPC, etc.) were invisible to
the caller. `disk_io::write` returned Ok, `install_patch` returned Ok,
`update()` returned `UpdateInstalled`, but `patches_state.json` was
left at 0 bytes. On the next launch, `load_patches_state` failed to
deserialize and fell back to default (`next_boot_patch: None`), so the
subsequent `checkForUpdate()` saw the server's patch as newly
installable and reported `UpdateStatus.outdated` despite the app
having "successfully" installed it moments earlier.
Change `disk_io::write` to:
- Write to a sibling `<file>.tmp` and atomically `rename` into place,
so `path` is never observed in a truncated/empty state by a
concurrent or post-crash reader.
- Explicitly unwrap the `BufWriter` via `into_inner()`, which calls
`flush_buf` and returns any I/O error as `IntoInnerError` instead
of dropping it on the floor.
- Clean up the temp file on failure.
Extract `serialize_and_flush` so the flush-error path is unit-testable
without filesystem tricks. Add three tests:
- temp file is cleaned up after a successful write
- a failed write preserves the existing file at \`path\`
- regression: \`serialize_and_flush\` surfaces inner-writer errors
(confirmed to fail on the pre-fix code)
* style: apply cargo fmt and rename roundtripped -> reloaded for cspell
* test: drop unreachable flush body in FailingWriter
* test: drop Result<()>/? boilerplate in new tests
Previously the macOS job relied on the runner's default target, which
produces a single architecture-dependent binary (currently uploaded as
patch-x86_64-apple-darwin.zip regardless of host). Explicitly build both
x86_64-apple-darwin and aarch64-apple-darwin so Apple Silicon hosts can
consume a native binary instead of hitting "Bad CPU type in executable"
when Rosetta is unavailable.
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.
`ShorebirdUpdater.update()` previously threw `UpdateException: No update
(noUpdate)` whenever the patch check returned no available update, even
though that is a successful outcome of calling `update()` — the app is
already running the latest patch.
The wrapper only returned normally on `SHOREBIRD_UPDATE_INSTALLED`;
every other status (including `SHOREBIRD_NO_UPDATE`, value 0) fell
through to the generic `throw UpdateException(...)` path, which is
why callers saw `UpdateException: No update (noUpdate)` in their
exception telemetry.
Treat `SHOREBIRD_NO_UPDATE` as a successful return alongside
`SHOREBIRD_UPDATE_INSTALLED`. No FFI change, safe against any engine
version (status code 0 is stable).
Fixesshorebirdtech/shorebird#3681
* test: add coverage for state recovery, download validation, rollback, and resume edge cases
Add 18 new unit tests across 4 test modules to increase confidence in
the updater's most sensitive code paths — areas that are hard to test
manually and where bugs could brick apps.
- state_recovery_tests: corrupt/missing/truncated state files, crash
during boot with missing artifacts, crash recovery event queuing,
client_id preservation across state corruption
- download_validation_tests: download failures, empty downloads,
non-zstd data, malformed server responses, network failures
preserving existing patch state
- rollback_unit_tests: rollback of non-existent patches, multi-patch
rollback
- resume_edge_case_tests: sidecar without partial file, server ignoring
Range header, failed download preserving sidecar for retry, resume
after failure
* fix: replace 'Reinit' with 'Reinitialize' for cspell
* fix: assert state files exist before corrupting them in tests
Ensures corruption tests fail loudly if the hardcoded file paths
(patches_state.json, state.json, patches/) don't match the actual
constants used by the updater, rather than silently passing.
* style: fix formatting and clippy warnings in new tests
- Apply cargo fmt to new test code
- Remove needless borrows on PATCH_BYTES (clippy)
* ci: speed up Rust CI builds with caching and prebuilt tools
The Windows builder takes ~6min vs ~3min on Ubuntu, largely because
cargo-llvm-cov is compiled from source on every run. This adds
Swatinem/rust-cache for build artifact caching and switches to
taiki-e/install-action for prebuilt cargo-llvm-cov binaries.
Also fixes the undefined `inputs.shell` references by hardcoding `bash`.
* fix: add Swatinem and taiki to cspell dictionary
* ci: remove rust-cache, no measurable benefit for this project
Investigated cache hit/miss across 3 CI runs. Even with warm cache
hits (macOS/Windows), build times were unchanged or slightly slower
due to cache download/extract overhead (~39MB) outweighing the small
dependency compile time savings.
* ci: remove redundant cargo build step
Clippy already compiles all targets, so the explicit cargo build step
was redundant. The test step (cargo llvm-cov) also does its own
instrumented build, so nothing depended on the build step's artifacts.
* ci: try larger Windows runner for faster Rust builds
Switch from windows-latest (4 vCPU) to windows-latest-large (8 vCPU)
to see if the extra cores meaningfully speed up Rust compilation.
Windows builds currently take ~2x longer than macOS/Ubuntu.
* ci: use Namespace Windows runner (8 vCPU) instead of GitHub-hosted
Switch to nscloud-windows-2022-amd64-8x16 to test whether doubling
the cores from 4 to 8 meaningfully speeds up Rust compilation on
Windows, which currently takes ~2x longer than macOS/Ubuntu.
* fix: add nscloud to cspell dictionary
* perf: add release profile to reduce binary size
Add workspace-level release profile with size optimizations:
- opt-level = "z" (optimize for size)
- lto = true (cross-crate link-time optimization)
- codegen-units = 1 (better whole-program optimization)
- strip = "debuginfo" (remove debug info, preserve C API symbols)
Reduces .text section by ~32% (1.9 MiB → 1.3 MiB). The .a file
grows due to LTO bitcode embedding, but the final linked binary
(libflutter.so) will be smaller when the engine consumes it.
* chore: add codegen and debuginfo to spell check dictionary
* ci: speed up Rust CI builds with caching and prebuilt tools
The Windows builder takes ~6min vs ~3min on Ubuntu, largely because
cargo-llvm-cov is compiled from source on every run. This adds
Swatinem/rust-cache for build artifact caching and switches to
taiki-e/install-action for prebuilt cargo-llvm-cov binaries.
Also fixes the undefined `inputs.shell` references by hardcoding `bash`.
* fix: add Swatinem and taiki to cspell dictionary
* ci: remove rust-cache, no measurable benefit for this project
Investigated cache hit/miss across 3 CI runs. Even with warm cache
hits (macOS/Windows), build times were unchanged or slightly slower
due to cache download/extract overhead (~39MB) outweighing the small
dependency compile time savings.
* style: fix clippy warnings and add clippy check to CI
Runs `cargo clippy --all-targets -- -D warnings` in the rust_crate CI
action to catch lint issues before they land.
Fixes: redundant field names, needless returns, unnecessary closures,
io_other_error, needless borrows, single_match, unnecessary_unwrap,
and adds missing safety docs.
* chore: add 'clippy' to cspell dictionary
* feat: add resumable downloads with streaming to disk
Previously, failed patch downloads were lost entirely — the updater
would re-download from scratch on every retry, creating a doom loop for
users on poor networks. Downloads were also fully buffered in memory.
This changes the download abstraction from returning bytes in memory
(`DownloadFileFn`) to streaming directly to disk with resume support
(`DownloadToPathFn`). The new signature accepts a `resume_from` byte
offset and the default implementation uses HTTP Range headers.
Key changes:
- DownloadToPathFn streams to file, sends Range header when resuming
- DownloadResult returns total_bytes and content_length from server
- DownloadState sidecar JSON tracks URL/patch/size for resume decisions
- compute_resume_offset detects valid partial downloads to resume
- Post-download size validation catches truncated downloads
- cleanup_download_artifacts removes compressed files + sidecars after
successful install (fixes pre-existing leak of download artifacts)
The fn-pointer signature maps directly to a future C callback for
platform-native download backends (iOS NSURLSession, Android
DownloadManager).
* fix: address self-review issues in download implementation
- Parse Content-Range header for 206 responses to get total file size
(reqwest's content_length() only returns the partial body size)
- Only append to existing file when server actually returns 206, not
just when resume_from > 0 (handles servers that ignore Range header)
- Remove incorrect resume_from offset addition to expected_size
- Clean up download artifacts on size mismatch before bailing
- Restore parent directory creation in download_to_path wrapper
* refactor: remove expected_hash from DownloadState
The hash stored in the sidecar was the *inflated* file hash from the
server response — it couldn't validate the compressed partial download
and was never used for resume decisions. The URL match already
determines whether to resume, and the real hash check happens after
inflate using the fresh server response. Simplifies the sidecar to
just url, patch_number, and expected_size.
* test: add coverage for resumable downloads + review fixes
- Add 12 new tests covering:
- compute_resume_offset: no sidecar, matching sidecar, mismatched URL,
empty file
- cleanup_download_artifacts: removes file+sidecar, noop when missing
- Integration: successful update cleans up artifacts
- Integration: partial download resumes via 206 with mockito
- Integration: URL change triggers fresh download
- parse_content_range_total: valid, missing header, unknown size (*)
- Extract parse_content_range_total as testable helper (was inline chain)
- Add expected_hash back to DownloadState — catches the case where a
patch is deleted and re-added with same number but different content
- Add "rsplit" to cspell config
* feat: add orphan cleanup for download directory + WriteFile comment
Scan the download directory before each download and remove any files
that don't belong to the current patch number. We own this directory
entirely, so anything from a prior patch, a crashed inflate (.full),
or an unrecognized file is safe to delete. This prevents gradual
accumulation of orphaned partial downloads over time.
Also adds a TODO comment explaining the reuse of WriteFile context for
seek operations (FileOperation lacks a SeekFile variant).
* test: add coverage for hash mismatch, patch number mismatch, corrupt sidecar
- compute_resume_offset_mismatched_hash: same URL but hash changed
(patch deleted and re-added), verifies fresh download
- compute_resume_offset_mismatched_patch_number: sidecar for different
patch number, verifies fresh download
- compute_resume_offset_corrupt_sidecar: garbage JSON in sidecar,
verifies graceful fallback to fresh download
* test: cover download size mismatch and unknown content-length paths
- update_fails_on_download_size_mismatch: mock returns content_length
that doesn't match total_bytes, verifies error + artifact cleanup
- update_succeeds_when_content_length_unknown: verifies the size
validation is skipped when content_length is None
* fix: replace fake hash strings to pass cspell
* chore: remove unnecessary TODO comment about FileOperation::SeekFile
* refactor: panic in test download mocks that should never be called
Tests where the download is never reached (patch check fails or no
patch available) now panic instead of returning dummy data, making it
explicit that the mock shouldn't be invoked.
* refactor: add UNEXPECTED_DOWNLOAD/UNEXPECTED_REPORT test constants
Shared panicking constants for test mocks that should never be called.
Tests use these by name instead of writing inline panic closures,
making intent clearer and avoiding uncoverable dead code in closures.
* test: add coverage for handle_download_result
Tests for the download-specific HTTP response handler:
- 200 OK: accepted
- 206 Partial Content: accepted (for resumed downloads)
- 500: rejected with error message
* fix: update missed download fn signatures in tests
Two test sites in updater.rs were not updated to the new
DownloadToPathFn 3-argument signature:
- set_noop_network_hooks in multi_engine_tests used old 1-arg closure
- update_starts_fresh_when_url_changes had unused patch_bytes variable
* test: add coverage for handle_download_result error branches
Mirror the existing handle_network_result_no_internet and
handle_network_result_unknown_error tests for the download variant.
These exercise the connection error and builder error paths in
handle_download_result that were previously uncovered.
* fix: adapt resumable downloads to ureq (post-rebase cleanup)
- Replace reqwest with ureq for download_to_path_default
- Remove handle_download_result (ureq's handle_network_result handles 206)
- Fix TempDir::new("prefix") → TempDir::new() for tempfile crate
- Remove unused Read/Write imports
Run `cargo update` to bump transitive dependencies, fixing 10 of 11
alerts (h2, ring, idna, mio, tokio, bytes, time, quinn-proto,
rustls-webpki, unsafe-libyaml).
Replace deprecated `tempdir` dev-dependency with `tempfile` to
eliminate the `remove_dir_all` vulnerability (the last alert).
* refactor: replace reqwest with ureq to reduce binary size
reqwest's blocking API is built on top of its async implementation,
pulling in tokio, hyper, futures, and ~84 other transitive dependencies
even though we only make simple synchronous HTTP calls.
ureq is a synchronous-only HTTP client that eliminates the async
runtime entirely. This reduces transitive dependencies from 227 to 143
and the linked dylib from 4.5 MB to 3.7 MB (-18%). The .a archive
drops from 28 MB to 26 MB, but real savings will be larger once
linked into libflutter with dead code stripping.
The network API surface is unchanged — three functions (patch check,
file download, event reporting) using POST/GET with JSON.
* chore: add ureq to spell check dictionary
* refactor: use into_body() instead of body_mut() where response is consumed
* fix: simplify network error matching to avoid fragile string checks
Consolidate HostNotFound, ConnectionFailed, and all Io errors into
a single network-error arm instead of pattern-matching on error
message strings that could change across OS versions or locales.
* chore: add TODO for misleading network error message
* test: test api calls
* chore: add docs
* chore: fix cspell
* fix: use no-op network hooks in multi_engine tests
Set no-op network hooks after init_for_testing so the fire-and-forget
thread spawned by report_launch_success completes instantly without
network I/O, preventing leaked threads from interfering with subsequent
serial tests that use mock servers.
Previously, checkForUpdate returned upToDate after a rollback because
the condition `next != null && current?.number != next.number` treated
a null next patch as "up to date". After a rollback, the Rust updater
correctly uninstalls the patch (next becomes null), but the app is still
running the rolled-back patch (current is non-null). The simplified
condition `current?.number != next?.number` correctly detects this
mismatch and returns restartRequired.
Fixes https://github.com/shorebirdtech/shorebird/issues/3206
* fix: improve inflate error handling and validate compressed patches
Addresses #2989: "pipe reader has been dropped" / "failed to fill whole
buffer" errors during patch inflation were masking the real root cause.
Three changes:
1. Join the decompression thread and propagate its error as the primary
failure, rather than fire-and-forget logging. The patching thread's
broken-pipe error is a side-effect, not the cause. Also drop the pipe
reader before joining to avoid deadlock when patching fails.
2. Validate the downloaded compressed patch (non-empty, valid zstd magic
bytes) before attempting decompression, so corrupt/truncated downloads
produce a clear error instead of cryptic pipe errors.
3. Log the download size in download_to_path to help diagnose truncated
downloads in the field.
* docs: expand comment on drop(fresh_r) to clarify deadlock risk
* feat: log app_id, patch number, and version before download
* test: add inflate tests for corrupt data and invalid magic
* test: cover decompression-error-as-primary-cause path in inflate
Uses a valid zstd frame followed by a corrupt second frame so that
bipatch::Reader::new succeeds but decompression fails midway, verifying
that the decompression error is reported as the primary cause.
Users sometimes gate app startup on checkForUpdate() or update()
completing (e.g. awaiting in initState before showing content), which
can cause the app to appear stuck on the splash screen when the
network is slow.
Add warning doc comments to both methods recommending the .then()
pattern for startup code, and update README examples to use .then().
Fixes https://github.com/shorebirdtech/shorebird/issues/3179
* feat: add enhanced error messages for file operations
Add a file_errors module that provides context-aware error messages
for file operations. When file operations fail, users now see:
- The specific operation that failed (create, read, write, rename, etc.)
- The full path involved
- Helpful hints about possible causes based on error type
- Android-specific hints for permission errors (SELinux, Work Profile,
MDM/Knox policies, app cloning features)
This helps diagnose issues like "Permission denied (os error 13)" by
indicating which operation failed and suggesting possible causes.
The scenario is:
1. User is running patch 2 (booted successfully, so last_booted_patch = 2)
2. While the app is still running, they call the check-for-update API
3. Patch 3 is downloaded and installed (next_boot_patch = 3)
4. Before the app restarts, they check again and patch 4 is available
5. The buggy code is supposed to delete patch 3 (never booted), but instead deletes patch 2 (the last known-good patch)
6. Patch 4 is set as next_boot_patch
If patch 4 boots fine, nobody notices. But if patch 4 fails to boot and the system tries to roll back to patch 2, those artifacts are gone.
* feat: move patch verification from boot time to install time
* feat: make it switchable
* chore: update comments
* fix: test invalid yaml
* chore: update readme
* feat: add more comments to readme
* doc: more readme updates
* chore: rename to patch_verification
Eric Seidel