-
Notifications
You must be signed in to change notification settings - Fork 46
add a first-run landing experience #73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
a5fdafe
f793c16
467c783
f0771b0
b9f8081
765c00c
33a0f53
33a72fb
911f7bf
1e93443
ba91185
2d08b19
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| use tauri::State; | ||
|
|
||
| use crate::services::installation_cohort::{ | ||
| InstallationCohort, InstallationCohortReadiness, InstallationCohortState, | ||
| }; | ||
|
|
||
| #[tauri::command] | ||
| pub async fn get_installation_cohort( | ||
| state: State<'_, InstallationCohortState>, | ||
| ) -> Result<InstallationCohort, String> { | ||
| let mut receiver = state.0.clone(); | ||
| loop { | ||
| let readiness = *receiver.borrow_and_update(); | ||
| if let InstallationCohortReadiness::Ready(cohort) = readiness { | ||
| return Ok(cohort); | ||
| } | ||
| if receiver.changed().await.is_err() { | ||
| return Ok(InstallationCohort::Unknown); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,235 @@ | ||
| use serde::{Deserialize, Serialize}; | ||
| use std::fs; | ||
| use std::io::Write; | ||
| use std::path::Path; | ||
| use tokio::sync::watch; | ||
|
|
||
| const MARKER_FILE_NAME: &str = "installation-cohort-v1.json"; | ||
| const MARKER_VERSION: u32 = 1; | ||
| const CURRENT_LAYOUT_DATABASE: &str = "berd.sqlite"; | ||
|
|
||
| #[derive(Clone, Copy, Debug, Deserialize, PartialEq, Eq, Serialize)] | ||
| #[serde(rename_all = "kebab-case")] | ||
| pub enum InstallationCohort { | ||
| FreshWithLandingV1, | ||
| EstablishedBeforeLandingV1, | ||
| Unknown, | ||
| } | ||
|
|
||
| #[derive(Clone, Copy, Debug, PartialEq, Eq)] | ||
| pub enum InstallationCohortReadiness { | ||
| Initializing, | ||
| Ready(InstallationCohort), | ||
| } | ||
|
|
||
| #[derive(Clone, Debug)] | ||
| pub struct InstallationCohortState(pub watch::Receiver<InstallationCohortReadiness>); | ||
|
|
||
| #[derive(Deserialize, Serialize)] | ||
| struct InstallationCohortRecord { | ||
| version: u32, | ||
| cohort: InstallationCohort, | ||
| } | ||
|
|
||
| pub fn layout_database_exists(app_data_dir: &Path) -> Result<bool, String> { | ||
| file_exists(&app_data_dir.join(CURRENT_LAYOUT_DATABASE)) | ||
| } | ||
|
|
||
| pub(crate) fn file_exists(path: &Path) -> Result<bool, String> { | ||
| match fs::metadata(path) { | ||
| Ok(metadata) => Ok(metadata.is_file()), | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 P2 · Ambiguous database paths become fresh (blocking) file_exists returns false for every existing path whose metadata is not a regular file. A directory, FIFO, or other unexpected entry at either database path is therefore treated exactly like NotFound, and startup may permanently publish the fresh cohort even though layout initialization or migration cannot safely use that path. This bypasses the otherwise fail-closed metadata-error behavior. User effect: After a filesystem conflict or malformed installation path, Berd can save the wrong permanent cohort and later force an established user through onboarding once the path problem is repaired. Recommended fix: Return an error for an existing non-file path and explicitly define symlink handling, so ambiguous evidence resolves to unknown without publishing a marker. Test: Create a directory at the current and legacy database paths and assert detection errors and no cohort marker is written; cover the intended symlink policy separately. |
||
| Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(false), | ||
| Err(error) => Err(format!("Failed to inspect {}: {error}", path.display())), | ||
| } | ||
| } | ||
|
|
||
| pub fn installation_cohort_channel() -> ( | ||
| watch::Sender<InstallationCohortReadiness>, | ||
| InstallationCohortState, | ||
| ) { | ||
| let (sender, receiver) = watch::channel(InstallationCohortReadiness::Initializing); | ||
| (sender, InstallationCohortState(receiver)) | ||
| } | ||
|
|
||
| pub fn initialize_installation_cohort( | ||
| app_data_dir: &Path, | ||
| current_layout_exists: Result<bool, String>, | ||
| legacy_layout_exists: Result<bool, String>, | ||
| ) -> Result<InstallationCohort, String> { | ||
| fs::create_dir_all(app_data_dir) | ||
| .map_err(|error| format!("Failed to create app data directory: {error}"))?; | ||
| let marker_path = app_data_dir.join(MARKER_FILE_NAME); | ||
|
|
||
| if marker_path.exists() { | ||
| let bytes = fs::read(&marker_path) | ||
| .map_err(|error| format!("Failed to read installation cohort marker: {error}"))?; | ||
| let Ok(record) = serde_json::from_slice::<InstallationCohortRecord>(&bytes) else { | ||
| return Ok(InstallationCohort::Unknown); | ||
| }; | ||
| if record.version != MARKER_VERSION || record.cohort == InstallationCohort::Unknown { | ||
| return Ok(InstallationCohort::Unknown); | ||
| } | ||
| return Ok(record.cohort); | ||
| } | ||
|
|
||
| let current_layout_exists = current_layout_exists?; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 P2 · Definitive established evidence is discarded (blocking) The current and legacy database probe Results are unwrapped sequentially. If either probe is Ok(true) but the other probe errors, initialization returns Unknown instead of using the already definitive evidence that an established database exists. For example, a valid current berd.sqlite is ignored when permissions prevent inspecting the obsolete legacy directory. User effect: An established user can be sent into first-run onboarding because an irrelevant secondary filesystem probe failed, even though Berd already found their existing database. Recommended fix: Aggregate the probes as three-state evidence: classify established if either is Ok(true), classify fresh only when both are Ok(false), and return unknown/error only when no positive evidence exists and at least one probe is uncertain. Test: Add cases for Ok(true) plus Err and Err plus Ok(true), asserting both classify and persist established; retain both-false and uncertainty-without-positive-evidence cases. |
||
| let legacy_layout_exists = legacy_layout_exists?; | ||
| let cohort = if current_layout_exists || legacy_layout_exists { | ||
| InstallationCohort::EstablishedBeforeLandingV1 | ||
| } else { | ||
| InstallationCohort::FreshWithLandingV1 | ||
| }; | ||
| persist_marker(&marker_path, cohort) | ||
| } | ||
|
|
||
| fn persist_marker(path: &Path, cohort: InstallationCohort) -> Result<InstallationCohort, String> { | ||
| let record = InstallationCohortRecord { | ||
| version: MARKER_VERSION, | ||
| cohort, | ||
| }; | ||
| let parent = path | ||
| .parent() | ||
| .ok_or_else(|| "Installation cohort marker has no parent directory".to_string())?; | ||
| let bytes = serde_json::to_vec(&record) | ||
| .map_err(|error| format!("Failed to serialize installation cohort marker: {error}"))?; | ||
| let mut temporary = tempfile::NamedTempFile::new_in(parent) | ||
| .map_err(|error| format!("Failed to create installation cohort marker: {error}"))?; | ||
| temporary | ||
| .write_all(&bytes) | ||
| .and_then(|_| temporary.as_file().sync_all()) | ||
| .map_err(|error| format!("Failed to write installation cohort marker: {error}"))?; | ||
|
|
||
| match temporary.persist_noclobber(path) { | ||
| Ok(_) => { | ||
| if let Err(error) = sync_parent_directory(path) { | ||
| log::warn!( | ||
| "Installation cohort marker was published but directory sync failed: {error}" | ||
| ); | ||
| } | ||
| Ok(cohort) | ||
| } | ||
| Err(error) if error.error.kind() == std::io::ErrorKind::AlreadyExists => { | ||
| read_published_cohort(path) | ||
| } | ||
| Err(error) => Err(format!( | ||
| "Failed to publish installation cohort marker: {}", | ||
| error.error | ||
| )), | ||
| } | ||
| } | ||
|
|
||
| fn read_published_cohort(path: &Path) -> Result<InstallationCohort, String> { | ||
| let bytes = fs::read(path) | ||
| .map_err(|error| format!("Failed to read published installation cohort marker: {error}"))?; | ||
| let record: InstallationCohortRecord = serde_json::from_slice(&bytes) | ||
| .map_err(|error| format!("Invalid published installation cohort marker: {error}"))?; | ||
| if record.version != MARKER_VERSION || record.cohort == InstallationCohort::Unknown { | ||
| return Err("Published installation cohort marker is unsupported".to_string()); | ||
| } | ||
| Ok(record.cohort) | ||
| } | ||
|
|
||
| #[cfg(unix)] | ||
| fn sync_parent_directory(path: &Path) -> Result<(), String> { | ||
| let parent = path | ||
| .parent() | ||
| .ok_or_else(|| "Installation cohort marker has no parent directory".to_string())?; | ||
| fs::File::open(parent) | ||
| .and_then(|directory| directory.sync_all()) | ||
| .map_err(|error| format!("Failed to sync installation cohort directory: {error}")) | ||
| } | ||
|
|
||
| #[cfg(not(unix))] | ||
| fn sync_parent_directory(_path: &Path) -> Result<(), String> { | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use tempfile::tempdir; | ||
|
|
||
| #[test] | ||
| fn classifies_and_persists_a_fresh_installation() { | ||
| let root = tempdir().unwrap(); | ||
| let first = initialize_installation_cohort(root.path(), Ok(false), Ok(false)).unwrap(); | ||
| assert_eq!(first, InstallationCohort::FreshWithLandingV1); | ||
|
|
||
| fs::write(root.path().join(CURRENT_LAYOUT_DATABASE), b"later").unwrap(); | ||
| let second = initialize_installation_cohort(root.path(), Ok(false), Ok(false)).unwrap(); | ||
| assert_eq!(second, InstallationCohort::FreshWithLandingV1); | ||
| } | ||
|
|
||
| #[test] | ||
| fn classifies_current_or_legacy_layouts_as_established() { | ||
| let current = tempdir().unwrap(); | ||
| fs::write(current.path().join(CURRENT_LAYOUT_DATABASE), b"existing").unwrap(); | ||
| assert_eq!( | ||
| initialize_installation_cohort(current.path(), Ok(true), Ok(false)).unwrap(), | ||
| InstallationCohort::EstablishedBeforeLandingV1 | ||
| ); | ||
|
|
||
| let legacy = tempdir().unwrap(); | ||
| assert_eq!( | ||
| initialize_installation_cohort(legacy.path(), Ok(false), Ok(true)).unwrap(), | ||
| InstallationCohort::EstablishedBeforeLandingV1 | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn concurrent_initializers_use_the_published_winner() { | ||
| use std::sync::{Arc, Barrier}; | ||
|
|
||
| let root = tempdir().unwrap(); | ||
| let path = Arc::new(root.path().to_path_buf()); | ||
| let barrier = Arc::new(Barrier::new(3)); | ||
| let handles = [false, false].map(|legacy_exists| { | ||
| let path = Arc::clone(&path); | ||
| let barrier = Arc::clone(&barrier); | ||
| std::thread::spawn(move || { | ||
| barrier.wait(); | ||
| initialize_installation_cohort(&path, Ok(false), Ok(legacy_exists)).unwrap() | ||
| }) | ||
| }); | ||
| barrier.wait(); | ||
|
|
||
| let cohorts = handles.map(|handle| handle.join().unwrap()); | ||
| assert_eq!(cohorts[0], InstallationCohort::FreshWithLandingV1); | ||
| assert_eq!(cohorts[1], cohorts[0]); | ||
| assert_eq!( | ||
| initialize_installation_cohort(&path, Ok(false), Ok(false)).unwrap(), | ||
| cohorts[0] | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn detection_failure_does_not_publish_a_fresh_marker() { | ||
| let root = tempdir().unwrap(); | ||
| assert!(initialize_installation_cohort( | ||
| root.path(), | ||
| Err("metadata unavailable".into()), | ||
| Ok(false), | ||
| ) | ||
| .is_err()); | ||
| assert!(!root.path().join(MARKER_FILE_NAME).exists()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn treats_an_unsupported_marker_as_unknown_and_preserves_it() { | ||
| let root = tempdir().unwrap(); | ||
| let marker = root.path().join(MARKER_FILE_NAME); | ||
| fs::write( | ||
| &marker, | ||
| br#"{"version":2,"cohort":"fresh-with-landing-v1"}"#, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| assert_eq!( | ||
| initialize_installation_cohort(root.path(), Ok(false), Ok(false)).unwrap(), | ||
| InstallationCohort::Unknown | ||
| ); | ||
| assert!(String::from_utf8(fs::read(marker).unwrap()) | ||
| .unwrap() | ||
| .contains("\"version\":2")); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 P2 · Cohort classification races migration (blocking)
Startup inspects current and legacy databases and permanently publishes the cohort marker before running legacy migration. Atomic no-clobber publication coordinates marker writers, but no inter-process lock covers the database observations and migration. A concurrently running Berd or legacy Goose Internal process can publish established data after inspection but before migration, allowing this process to persist fresh-with-landing-v1 and then copy/open established data.
User effect: An installation with established data can be permanently treated as new and unexpectedly routed through onboarding because of launch timing between processes.
Recommended fix: Serialize classification and legacy migration with an inter-process startup/migration lock. Under the lock, inspect pre-migration evidence, perform migration, and publish the marker only after the evidence and resulting layout are stable.
Test: Add a lock-boundary or multi-process test that pauses after an absent observation, creates a valid legacy layout from another writer, resumes startup, and asserts the final marker is established rather than fresh.