main
18 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 nickshorebird
|
a591b7f6b9 |
feat(ci): adopt shorebird_ci workflow w/ required aggregator (#359)
ci / ✅ Semantic Pull Request (push) Has been cancelled
ci / 🔤 Check Spelling (push) Has been cancelled
ci / 👀 Detect Changes (push) Has been cancelled
Shorebird CI / changes (push) Has been cancelled
Shorebird CI / CSpell (push) Has been cancelled
ci / 🦀 Build ${{ matrix.crate }} (${{ matrix.os }}) (push) Has been cancelled
ci / 🎯 Build ${{ matrix.package }} (push) Has been cancelled
ci / ci (push) Has been cancelled
Shorebird CI / shorebird_code_push (push) Has been cancelled
Shorebird CI / shorebird_code_push_example (push) Has been cancelled
Shorebird CI / required (push) Has been cancelled
* feat(ci): add shorebird_ci-generated workflow w/ required aggregator Generated w/ `shorebird_ci generate --style static --required`. Covers the `shorebird_code_push` Dart package; Rust crates remain on main.yaml. * fix(ci): add dorny, felangel, subpackages to cspell dictionary |
||
|
Eric Seidel
|
90c349287d |
refactor: per-patch lifecycle state machine (#352)
* refactor: introduce per-patch lifecycle state machine (types + persistence)
First slice of shorebirdtech/shorebird#3737 — replace the scattered
storage of per-patch state across download_state.rs sidecars, bare
files in downloads/, and PatchesState fields (next_boot_patch,
last_booted_patch, known_bad_patches) with a single per-patch
state.json driven by an explicit state machine.
This commit only adds the types and the storage layer; nothing wires
into update_internal or report_launch_* yet. Subsequent commits will
build out transitions and migrate the call sites.
States:
- Downloading { url, hash, signature, partial_size }
- Downloaded { url, hash, signature, size }
- Installed { hash, signature, size }
- Bad { reason, hash?, signature?, size? } // tombstone
Operations:
- mark_bad(n, reason) — sugar over write Bad{} + cleanup
- cleanup(n) — state-aware: keeps Bad tombstone, else forgets dir
Per-release pointers (next_boot, last_booted, currently_booting,
boot_started_at) move to a separate pointers.json holding patch
numbers; metadata lives once per patch in state.json instead of
duplicated across pointers.
Persistence rides on the existing atomic disk_io::write (sibling-
write + rename) so partial writes can't leave torn files.
* refactor: add download/install/boot transitions to lifecycle
Extends the lifecycle module with the methods update_internal and
report_launch_* will need:
- decide_start(n, url, hash) → DownloadAction (Fresh, Resume, Complete, Skip)
- record_download_started / _complete
- record_install_complete (transitions Downloaded → Installed,
removes the now-unneeded compressed download file)
- promote_to_next_boot (transitions a freshly Installed patch into
pointers.next_boot, retiring any unbooted predecessor)
- record_boot_start / _success / _failure
- detect_boot_crash_on_init (handles the breadcrumb left when a
prior process crashed during boot)
- validate_next_boot_patch (size + signature checks; marks
Bad{ValidationFailed} on failure)
- recompute_next_boot
Nothing wires into the existing updater.rs / report_launch_* yet —
those changes follow in the cutover commit.
* refactor: replace UpdaterState's PatchManager backend with PatchLifecycle
Drops the patch_manager dependency from updater_state.rs. UpdaterState
now owns a PatchLifecycle directly, with all patch-related methods
delegating to it:
install_patch → write Installed state + promote to next_boot
next_boot_patch → pointers.next_boot_patch + installed_artifact_path
is_known_bad_patch → read_state matches Bad
uninstall_patch → cleanup + recompute_next_boot
record_boot_* → lifecycle's boot transitions
validate_next_boot_patch → lifecycle's signature/size check
UpdaterState's own state.json shrinks to {client_id, release_version,
queued_events}. Per-release per-patch state lives entirely in the
lifecycle (pointers.json + patches/{N}/state.json).
Other notable behavior changes:
- record_boot_failure_for_patch no longer requires
currently_booting_patch to match. Matches the prior PatchManager
semantics: clear breadcrumb, mark Bad{BootCrash}, recompute.
- recompute_next_boot leaves a valid Installed next_boot_patch alone
instead of promoting last_booted over it. Without this, processing
server rollbacks would clobber a freshly-installed newer patch.
Tests in updater.rs that asserted file paths (patches_state.json) update
to the new layout (pointers.json). The MockManagePatches-backed unit
tests in updater_state.rs are gone — replaced by direct end-to-end tests
that exercise PatchLifecycle through UpdaterState.
patch_manager.rs is still on disk and compiled (nothing references it
from cache/mod.rs anymore) — deletion happens after the updater.rs
cutover.
* refactor: cut update_internal over to PatchLifecycle
Replaces the download_state.rs sidecar machinery and the bespoke
DownloadStartState / should_install_patch helpers in update_internal
with calls into PatchLifecycle.
Concrete changes:
- Drops compute_resume_offset / determine_download_start_state /
DownloadStartState — replaced by PatchLifecycle::decide_start
returning DownloadAction.
- Drops should_install_patch / ShouldInstallPatchCheckResult —
folded into the same DownloadAction match. KnownBad → UpdateIsBadPatch,
AlreadyInstalled → NoUpdate, the rest proceed.
- Drops install_downloaded_patch's bespoke "stage in downloads/, rename
on install" choreography. The lifecycle owns the per-patch directory;
inflate writes directly into patches/{N}/dlc.vmcode and the
Downloaded → Installed transition removes the now-unneeded compressed
download file.
- Drops cleanup_download_artifacts and clean_download_dir. mark_bad
handles tombstone-aware cleanup for failures, and the lifecycle owns
its directory.
- Marks the patch Bad{InvalidPatchBytes} when inflate fails and
Bad{InstallHashMismatch} when check_hash fails. Subsequent attempts
short-circuit at decide_start (Skip(KnownBad)) instead of
re-downloading and re-failing on every cycle. This is the marks-bad-
on-install-failure behavior we deferred from #351.
- Preserves the explicit "server-side Content-Length vs total_bytes"
mismatch check — surfaces the contract violation directly instead of
obliquely via inflate failure.
decide_start consults the on-disk download file as the source of truth
for "how many bytes do we have so far." The denormalized partial_size
in PatchState::Downloading is kept for diagnostics/serde stability but
not consulted for the resume offset; this matches the prior
file-size-on-disk behavior and avoids needing to update state.json
mid-stream from the network layer.
For Downloading / Downloaded states where the OS evicted the artifact
file out from under us (e.g. iOS code-cache eviction), decide_start
falls through to Fresh so the next attempt re-downloads from scratch.
The failing-test fixes update file paths and pre-staged state from the
old layout (downloads/{N} + downloads/{N}.download.json) to the new
layout (patches/{N}/download + patches/{N}/state.json). Several tests
that targeted the now-deleted helpers directly are removed; their
behavior is covered by the new lifecycle unit tests.
* refactor: delete patch_manager.rs and download_state.rs
Both modules are now subsumed by PatchLifecycle:
- patch_manager.rs (~1600 lines) managed the per-release `patches/`
tree, the `next_boot_patch` / `last_booted_patch` /
`currently_booting_patch` / `known_bad_patches` fields of
`PatchesState`, and the fall-back-from-bad-patch logic. All of
this is now in lifecycle.rs split between `PatchLifecycle`
(per-patch state.json) and `ReleasePointers` (a single
pointers.json).
- download_state.rs (~125 lines) wrote the per-download sidecar
JSON. Now folded into PatchState::Downloading /
PatchState::Downloaded variants in state.json, owned by the
lifecycle module.
The MockManagePatches / ManagePatches trait machinery in
updater_state.rs's tests goes away with patch_manager. The lifecycle
operations are tested directly in cache::lifecycle::tests against a
real on-disk filesystem under TempDir, which catches issues the
mocks couldn't (e.g. file-existence cross-checks in `decide_start`).
The cargo workspace now has 212 passing tests vs. 257 before, the
delta being the patch_manager unit tests whose subjects no longer
exist. End-to-end coverage in `updater.rs::tests` is unchanged and
all green.
* refactor: address self-review feedback on lifecycle PR
Nine fixes from the self-review pass on shorebirdtech/updater#352:
- Drop `partial_size` from PatchState::Downloading. The field was
misleading (decide_start reads from disk, not from the recorded
value) and unused. record_download_started loses its 5th arg.
- Gate `UpdaterState::install_patch` to `#[cfg(test)]`. Production
no longer routes through it; only test_utils and the updater_state
tests do. The gate makes the divergence intentional and prevents
future production callers.
- install_patch defensively removes any prior `dlc.vmcode` before
rename so behavior is OS-agnostic (POSIX rename overwrites silently;
Windows fails). Also mirrors record_install_complete's cleanup of
a stale `download` file in the patch dir.
- Document on `lifecycle()` / `lifecycle_mut()` that the direct
accessors are intentional — wrapping every transition would be
churn for no reader benefit.
- recompute_next_boot now clears `last_booted_patch` when its
on-disk record is gone (Unknown), so pointers.json doesn't
accumulate references to nothing. A `Bad` last_booted is left
alone — that's a useful breadcrumb and recompute simply doesn't
promote it.
- Promote `download_artifact_path` and `installed_artifact_path` to
methods on PatchLifecycle for symmetry with state_path /
pointers_path. Updates all call sites.
- Document mark_bad-on-Bad merge semantics: latest reason wins,
other fields preserved. Hypothetical in practice but no longer
silent.
- Add tests for recompute_next_boot's stale-pointer clearing
(Unknown → cleared, Bad → kept). Brings the suite to 214 tests.
The mark_bad-cleanup-stale-bytes recovery path I flagged was already
covered by `cleanup_on_bad_patch_keeps_tombstone` — no new test needed.
* refactor: undo two over-corrections from the prior self-review fix
Two issues introduced by 07ea84a that the second self-review caught:
- update_internal computed download_path / installed_path via
`with_state(...).lifecycle().download_artifact_path(n)`. That
routed a pure-function path computation through a state read,
serializing it against other state operations and adding a disk
read per call. Restore the free `download_artifact_path` /
`installed_artifact_path` functions as the canonical entry point;
the methods on PatchLifecycle are kept as thin wrappers for
callers that already hold a lifecycle handle.
- install_patch's "defensive remove_file before rename" added a
TOCTOU window between the exists() check and the rename for no
real benefit — install_patch is now `#[cfg(test)]`-only, the
Windows-rename concern was theoretical for tests, and POSIX rename
atomically overwrites. Drop the explicit remove_file.
* ci: fix warnings-as-errors and cspell
Three CI failures from the prior push:
- `Context` and `bail` imports in updater_state.rs are only used
inside the `#[cfg(test)] install_patch` helper. Moved them under
`#[cfg(test)]` so non-test builds don't see unused imports.
- `PathBuf` import in updater.rs became unused after switching the
download/install paths to take `Path::new(&config.storage_dir)`
directly. Dropped.
- `FileOperation::DeleteDir` is no longer constructed by production
code (was used by the deleted patch_manager.rs). Mirrored the
existing `#[allow(dead_code)]` annotation already on `DeleteFile`
rather than removing the variant — the format/handling code for
it is still useful for any future code that needs to surface a
"delete dir failed" error.
CSpell additions: `unparseable`, `tombstoned`, `roundtrips` — words
introduced by the lifecycle module's docs and test names.
* ci: gate PathBuf import to platforms that actually use it
The previous fix removed `PathBuf` from the top-level use, which
broke the lib build on non-android non-test platforms (macOS,
Linux, Windows) where `libapp_path_from_settings` uses `PathBuf`.
That function is itself gated to `cfg(not(any(target_os = \"android\",
test)))`, so the PathBuf import needs the same gate.
Restoring it as a cfg-gated separate import — keeps the lib build
green on all three runners and keeps the lib-test build clean
under -D warnings.
* test: cover the gaps surfaced by the coverage report
Adds 7 tests targeting branches that were uncovered:
- mark_bad_from_downloading_records_partial_file_size: the
Downloading source state in mark_bad reads bytes-on-disk for
the recorded `size` field; the existing tests only covered
Installed → Bad.
- mark_bad_overwrites_reason_when_already_bad: documented merge
semantics (latest reason wins, other fields preserved) now
has a test backing the doc.
- validate_next_boot_patch_marks_bad_when_artifact_missing: the
"Installed state but dlc.vmcode is gone" path. Existing test
covered size mismatch; this covers the missing-file branch.
- validate_next_boot_patch_marks_bad_when_pointer_targets_non_installed:
the case where the next_boot pointer references a state other
than Installed (state.json + pointers can diverge through
corruption).
- install_patch_install_only_{accepts_valid,rejects_missing,rejects_bad}_signature:
cover the InstallOnly verification path in
UpdaterState::install_patch using the existing test keypair
from signing.rs.
Coverage improvements:
- cache/lifecycle.rs: 94.65% → 96.06% lines, 98.72% → 100% fns
- cache/updater_state.rs: 94.34% → 96.94% lines, 95.65% → 98% fns
- total: 94.70% → 95.11% lines
214 → 221 tests, all passing under -D warnings.
* ci: add 'keypair' to cspell dictionary
* test: audit deleted patch_manager tests; restore strict checks
Goes through every test deleted with patch_manager.rs (~40) and either
maps it to a new test (with a `Ports patch_manager.rs::mod::name`
comment) or adds a port. Three behavioral regressions caught and fixed:
- record_boot_start now requires next_boot_patch to match the arg.
Old PatchManager had this defensive check; my refactor dropped it.
Carries forward the engine-vs-state agreement guard.
- Added strict-mode signature tests for validate_next_boot_patch
(valid, missing, invalid, no public key, bad public key). Ports
five `validate_next_boot_patch_tests::strict_mode_*` tests.
- Added InstallOnly + no public_key test (ports
add_patch_tests::install_only_succeeds_with_any_signature_if_no_public_key).
Plus two scenario tests that didn't have direct coverage:
- rolled_back_patch_not_resurrected_when_replacement_fails: ports
fall_back_tests::rollback_then_failed_replacement_does_not_resurrect_rolled_back_patch
against the new state-based fallback (cleanup forgets, recompute
won't promote a None state).
- validate_then_promote_catches_corrupted_last_booted: ports
validate_next_boot_patch_tests::does_not_fall_back_to_last_booted_patch_if_corrupted
with a note: new code catches the corruption at the next validate
pass instead of at fall-back time, but the user-visible outcome
(boot the base release) is the same.
Test helper split: `install_state` (writes Installed without touching
pointers) vs callers explicitly setting pointers. Avoids the prior
helper's auto-promote masking pointer-management behavior the tests
were trying to exercise.
Tests deliberately not ported (with reasoning):
- debug_tests::manage_patches_is_debug, patch_manager_is_debug:
Trait-Debug tests for types that no longer exist. Replaced
implicitly by `#[derive(Debug)]` on PatchLifecycle.
- fall_back_tests::succeeds_if_deleting_artifacts_fails: needs
filesystem fault injection that wasn't worth the test
infrastructure to bring back.
- record_boot_success_for_patch_tests::deletes_unrecognized_directories_in_patches_dir:
behavior change — new cleanup_older_than skips non-numeric
directory names rather than deleting them. Defensive; the prior
`rm -rf` was overly aggressive.
Coverage: 222 → 230 tests, all green under -D warnings.
* test: port the two patch_manager tests I'd handwaved
Both turned out trivial to port — neither was actually doing fault
injection, just exploiting graceful-failure paths.
- record_boot_success_deletes_unrecognized_directories_in_patches_dir:
pre-creates a junk dir and a stray file in patches/ before the
install/boot, asserts they're gone after record_boot_success.
Restores the prior `cleanup_older_than` behavior of removing
non-numeric entries — we own patches/, so anything not named like
a patch number is corruption to sweep up.
- record_boot_failure_succeeds_if_artifact_dirs_are_already_gone:
pre-deletes both patch dirs and verifies record_boot_failure still
completes correctly (mark_bad recreates the dir for the tombstone;
cleanup paths are graceful when the dir is missing).
Reverts the prior behavior change in cleanup_older_than that skipped
non-numeric entries instead of deleting them. The "defensive" framing
was wrong — we own patches/, anything unexpected there came from a
different version of our code or actual corruption, and leaving it
behind is the wrong default.
* test: port the remaining patch_manager tests with real coverage gaps
- validate_next_boot_strict_mode_succeeds_with_valid_signature: ports
`patch_manager.rs::validate_next_boot_patch_tests::strict_mode_succeeds_with_valid_signature_at_boot_time`.
Required reusing the prior test fixture's INFLATED_PATCH_HASH /
INFLATED_PATCH_SIGNATURE constants (a real signature for the
1-byte file content `b"1"`, matching TEST_PUBLIC_KEY's private
half).
- record_boot_failure_keeps_last_booted_pointer_when_failed_patch_was_last_booted:
ports
`patch_manager.rs::record_boot_failure_for_patch_tests::preserves_last_booted_patch_on_failure_but_marks_bad`.
The new behavior is similar but more explicit: last_booted's
*pointer* is preserved (with the patch now in Bad state); the
underlying intent — "we still know what last booted, even after
that patch failed" — survives.
Test helper split: `install_signed` (failure paths, mismatched hash
OK) vs `install_with_valid_signature` (happy path, real fixture).
Now every behavioral test from the deleted patch_manager.rs has a
corresponding new test with a porting comment. Two trivial ones not
ported: the `Debug` impl tests for traits/types that no longer exist.
236 tests, all green under -D warnings.
* refactor: restore separate download directory under OS cache root
Reverts an unintended policy change in the cutover: my refactor moved
compressed download bytes from `{code_cache_dir}/downloads/{N}` (the
OS-managed cache dir, evictable under storage pressure) into
`{storage_dir}/patches/{N}/download` (persistent app storage). That
made downloads count against persistent storage and survive in iCloud
backups — neither of which we want for transient bytes.
Restoring the original split:
- `{state_root}/patches/{N}/{state.json,dlc.vmcode}` — persistent
state and the installed artifact.
- `{download_root}/{N}` — flat, in the OS cache dir.
`PatchLifecycle::load_or_default` now takes both roots. The two
artifact-path helpers each route to their respective root, the
free-function variants used by `update_internal` now take the right
root, and `mark_bad`/`cleanup`/`record_install_complete` clean both
locations as appropriate.
`UpdaterState::create_new_and_save` also wipes `download_dir` on
release-version change, in addition to the persistent patches/ tree.
`update_internal` and tests updated to use the right root for each
file type. Test fixtures pass `tmp.path().join("downloads")` as the
download root (separate subdir of the same TempDir).
Net change in test surface: moved a handful of `patch_dir/download`
references to `downloads_dir/N`, no behavioral changes to tests
themselves. 236 tests pass under -D warnings.
* test: cover the new download_root cleanup paths
Self-review caught two gaps in the download-root split:
- release_version_change_wipes_download_dir: the cache-rooted
download dir should be wiped along with the persistent patches/
tree on a release-version mismatch. Adds explicit coverage —
the code path was already in create_new_and_save but not
directly tested.
- record_boot_success_promotes_and_cleans_older: extends the
existing test to drop a stale download in download_root
alongside an older patch's state, asserts that
cleanup_older_than's chain (cleanup → forget_dir) deletes both
roots' artifacts.
Also adds a comment on cleanup_older_than noting that it only
walks the persistent patches/ tree — orphan downloads (no
state.json) would persist until the OS evicts them. Noted as a
known limitation, not blocking.
* refactor: targeted wipe + legacy patches_state.json + orphan-download walk
Two fixes to the release-version-change wipe and one to the boot-
success cleanup walk.
- The "wipe everything in cache_dir" approach broke 27 tests whose
setup uses cache_dir as the test temp dir (with base.apk and
libapp.so as siblings). Production gives shorebird a dedicated
subdir but the API doesn't enforce that. Switched to a targeted
wipe with a documented allowlist of paths shorebird has ever
written under cache_dir: `SHOREBIRD_OWNED_PATHS`.
- Added `patches_state.json` to the wipe list — that's the
legacy file from the prior `PatchManager` implementation, and
devices upgrading through this PR would otherwise orphan it.
New test `release_version_change_wipes_legacy_patches_state_json`
covers it.
- `cleanup_older_than` now also calls `cleanup_orphan_downloads`,
which walks `download_root/` and removes any file that doesn't
correspond to a patch in `Downloading`/`Downloaded` state. We own
the cache root and shouldn't rely on OS eviction. Catches the
"state.json gone but download lingered" case the prior comment
handwaved.
Adding a new file under cache_dir means adding it to
SHOREBIRD_OWNED_PATHS — small bookkeeping cost, but it lets us
keep cache_dir co-tenant with embedder-provided files.
* test: cover all four branches of cleanup_orphan_downloads
The new orphan-download sweep distinguishes four cases but only the
"older patch via cleanup chain" branch was being exercised. Single
test now drops one of each kind of file into download_root before a
boot success and asserts which survive:
- orphan (numeric, no state.json): removed
- stale (numeric, state is Installed): removed
- non-numeric name: removed
- live (numeric, state is Downloading): preserved
* ci: add 'embedder' to cspell dictionary
* refactor: drop Installed.hash, fix mark_bad ordering, address bdero review
bdero's review on #352 surfaced six items worth landing inline:
1. record_boot_failure used to clear `currently_booting_patch` before
`mark_bad`. A crash between the two left the patch `Installed` with
no breadcrumb, so `detect_boot_crash_on_init` wouldn't fire next
boot — the patch silently retried. Flipped: mark_bad first, then
clear. mark_bad on Bad is idempotent, so the worst case is a
redundant tombstone-rewrite on next init.
2. Drop the `hash` field from `PatchState::Installed`. It was recorded
at install but never read at boot — Strict-mode validation
recomputes the hash from the artifact's bytes and feeds it into
`check_signature`. A hash that lives only on disk can't be trusted
as a security input; deleting the field removes the temptation.
`Downloading.hash`/`Downloaded.hash` stay as comparators against
the server's freshly-delivered hash (a tampered on-disk hash there
just causes a redownload).
3. Port the customer-scenario test from #356 into `c_api/mod.rs`:
server rolls patch 1 back, then forward again with the same number
and hash. Pre-lifecycle this was permanently dropped via
`known_bad_patches`. Post-lifecycle `cleanup` is state-aware and
forgets the patch entirely on server-driven rollback, so the
rollforward installs cleanly. End-to-end coverage of the regression
#356 hotfixed.
4. Add a TODO + target version on the `patches_state.json` entry in
`SHOREBIRD_OWNED_PATHS` so the legacy wipe doesn't outlive its
usefulness.
5. Document that `running_patch` lives in `config.rs` (session-scoped),
not in this module — saves a future grep.
6. Clarify `recompute_next_boot`'s fallback policy: it's
fall-back-to-`last_booted_patch`-only, not fall-back-to-anything-
bootable. A fresh release whose first Installed patch fails
validation goes to base, even if older patches sit in `patches/`.
cargo test: 242 passed (was 241).
|
||
|
Eric Seidel
|
96fd32796e |
feat: stage 2 integration tests — fake patch server + golden path + rollback regression (#355)
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. |
||
|
Eric Seidel
|
8649c75206 |
perf: mmap libapp.so out of the APK instead of buffering in RAM (#354)
* 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 |
||
|
Eric Seidel
|
8072ed9ad7 |
feat: stage 1 integration test harness (#353)
* 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. |
||
|
Eric Seidel
|
a1f2a9b8bc |
ci: speed up Rust CI builds (#327)
* 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 |
||
|
Eric Seidel
|
07fa11c9d9 |
perf: add release profile to reduce binary size (#328)
* 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 |
||
|
Eric Seidel
|
0af491b628 |
ci: speed up Rust CI builds with prebuilt tools (#325)
* 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. |
||
|
Eric Seidel
|
50954da68a |
style: fix clippy warnings and add clippy check to CI (#321)
* 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 |
||
|
Eric Seidel
|
463ace0c56 |
feat: add resumable downloads with streaming to disk (#313)
* 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
|
||
|
Eric Seidel
|
c6647a2dfe |
refactor: replace reqwest with ureq to reduce binary size (#317)
* 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 |
||
|
Eric Seidel
|
4ff2839cdb |
feat: add tests and docs for boot state machine (#309)
* 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. |
||
|
Eric Seidel
|
6d0e4a1193 |
chore(deps): bump Rust and Dart dependencies (#315)
* chore(deps): bump Rust and Dart dependencies Bump Rust dependencies in library/ and patch/: - comde: 0.2.3 → 0.3.1 (library), 0.2.3 → 0.3.0 (patch) - zip: 0.6.4 → 3.0.0 (breaking: FileOptions → SimpleFileOptions) - android_logger: 0.13.0 → 0.15.0 - mockall: 0.12.1 → 0.13.1 - serial_test: 2.0.0 → 3.2.0 - cbindgen: 0.24.0 → 0.28.0 Bump Dart dev dependency in shorebird_code_push/: - ffigen: upper bound <17.0.0 → <19.0.0 Updated zip API usage (FileOptions → SimpleFileOptions) and adjusted test assertion for changed error message. Binary size impact (macOS release, arm64): - libupdater.a: +83 KB (+0.28%) - libupdater.dylib: +34 KB (+0.75%) Closes #206, #271, #273. * chore: add EOCD to cspell dictionary The zip 3.0 crate changed its error message to reference "EOCD" (End of Central Directory), which cspell doesn't recognize. |
||
|
Brandon DeRosier
|
eeec42efb7 |
feat: add enhanced error messages for file operations (#310)
* 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. |
||
|
Felix Angelov
|
b2fbf7c3ee | chore: various platforms updates (#295) | ||
|
Bryan Oltman
|
a6c761f50f |
fix: build updater with static runtime to prevent flutter engine build linking errors (#254)
* fix: build updater with static runtime to prevent flutter engine build linking errors * Update library/.cargo/config.toml Co-authored-by: Eric Seidel <eric@shorebird.dev> * cspell --------- Co-authored-by: Eric Seidel <eric@shorebird.dev> |
||
|
Eric Seidel
|
6298d37d86 |
chore: add cspell checking and make pass (#244)
* chore: add cspell checking and make pass * chore(shorebird_code_push): minor improvements to example (#242) * chore(shorebird_code_push): v2.0.2 (#243) * chore: fix cspell --------- Co-authored-by: Felix Angelov <felix@shorebird.dev> |
||
|
Eric Seidel
|
2ae6afbb65 |
fix: unbreak web build for 2.x (#241)
* fix: unbreak web build * chore: add cspell config |