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: 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.
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.
* 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(shorebird_code_push): track support
* cleanup and add todos
* docs
* run ffigen
* use c_char instead of char
* Add channel support
* tests
* tests
* update podfile.lock
* Update example to include tracks selector
---------
Co-authored-by: Bryan Oltman <bryan@shorebird.dev>
Co-authored-by: Bryan Oltman <bryanoltman@gmail.com>
I just ran `cargo clippy -- -W clippy::pedantic` and fixed things.
These are more invasive that the default set and the remaining
warnings are mostly about our (abysmal) public docs missing
Error and Panic sections to explain errors and panicks.
* feat: add support for parsing auto_update from shorebird.yaml
The actual support will be in the C++ engine, but this keeps
all yaml parsing in the Rust code for simplicity.
---------
Co-authored-by: Felix Angelov <felix@shorebird.dev>
* Make our rust tests use per-thread config rather than global static
This lets our unit tests run in parallel. I also moved our "integration"
tests back to be unit tests for now.
This also exposes a network mocking abstraction.
I've added an incomplete test of applying a full patch. Still
needs some work to complete.
* Finish making the patch success test work
* Changed our test config to use println instead of info/error, etc. so that tests show output on failure (there might be a better way?)
Unfortunately macro_use (which is the way we were causing error!, info! etc to appear everywhere with only one global import before, only works on crates not on std, so I had to be explicit with my imports per-file.
* Add a function to build a fake zip file, because that's what the updater currently expects. Better would be for us to move to a AssetManager system I suspect, then the updater would ask the asset manager for the libapp.so and we'd return it instead of having to go through writing out a zip file.
* Added a string_patch.rs tool and shared code between that and the patch tool. This made it possible/easy for me to generate the necessary binary patches as well as needed hashes to pretend to be the patch server.
* Added one simple test of the patch tool core logic.
In total we're now above 80% coverage of our rust code after this.
* remove stray comment
* feat: Add start_updater_thread to update off the main thread
This makes it so that clients can easily not block when wanting to
queue an update.
I have a separate patch which updates the Engine to use this new
API.
I also needed to split the concept of the "next_boot" patch
from the "current_boot" patch, previously refered to as
"current" or "active" patch. This required adding a
report_launch_start api to let the updater library know
when to set current_boot patch from next_boot.
I also removed the rust updater/cli in this as well as the
vmpath argument to init.
I also exposed the report_launch_success api, but its not yet
used by the Engine.
I renamed report_failed_launch to report_launch_failure to match
report_launch_start which I introduced.
* Update naming per comments from Felix.
Also added a helper for char* allocation (not sure if it's better).
* Update library/src/updater.rs
---------
Co-authored-by: Felix Angelov <felix@shorebird.dev>
This was needed so that it could be called from Dart as well as flutter_main/C++.
It turns out flutter_main does not run on the "ui thread", so when Dart was calling
into the updater it would panic due to thinking the updater (which was using a thread local) was not yet initialized.
Also added the log-panics crate on Android so that panics appear in adb logcat.
Eric Seidel