Skip to content

Commit cc0c8a5

Browse files
committed
Reset stale fetching states on app launch
Add `reset_stale_fetching_states_internal` to `WpApiCache` that resets `FetchingFirstPage` and `FetchingNextPage` states to `Idle` after migrations. This prevents perpetual loading indicators when the app is killed during a fetch operation. The reset runs in `perform_migrations()` because `WpApiCache` is typically created once at app startup, unlike `MetadataService` which is instantiated per-service. Changes: - Add `reset_stale_fetching_states_internal()` with comprehensive docs - Call from `perform_migrations()` after migrations complete - Add session handover document for MetadataService implementation
1 parent 8fbd067 commit cc0c8a5

File tree

2 files changed

+181
-1
lines changed

2 files changed

+181
-1
lines changed
Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
# MetadataService Session Handover
2+
3+
## Completed Work
4+
5+
### Phase 1: Database Foundation (wp_mobile_cache) ✅
6+
- Added `DbTable` variants: `ListMetadata`, `ListMetadataItems`, `ListMetadataState`
7+
- Created migration `0007-create-list-metadata-tables.sql` with 3 tables
8+
- Implemented `ListMetadataRepository` with full CRUD + concurrency helpers
9+
- 31 tests covering all repository operations
10+
11+
### Phase 2: MetadataService (wp_mobile) ✅
12+
- Created `MetadataService` wrapping repository with site-scoped operations
13+
- Implements `ListMetadataReader` trait for compatibility with existing code
14+
- 15 tests covering service operations
15+
16+
### Phase 3: Integration (partial) ✅
17+
- Added `metadata_service` field to `PostService`
18+
- Added `sync_post_list()` method for database-backed sync orchestration
19+
- Extended `SyncResult` with `current_page` and `total_pages` fields
20+
- Preserved existing in-memory `metadata_store` for backwards compatibility
21+
22+
## Commits
23+
24+
| Commit | Description |
25+
|--------|-------------|
26+
| `3c95dfb4` | Add database foundation for MetadataService (Phase 1) |
27+
| `e484f791` | Add list metadata repository concurrency helpers |
28+
| `3c85514b` | Add MetadataService for database-backed list metadata |
29+
| `5c83b435` | Integrate MetadataService into PostService |
30+
| `7f2166e4` | Update MetadataService implementation plan with progress |
31+
32+
## Key Files
33+
34+
- `wp_mobile_cache/src/list_metadata.rs` - Structs and `ListState` enum
35+
- `wp_mobile_cache/src/db_types/db_list_metadata.rs` - Column enums, `from_row` impls
36+
- `wp_mobile_cache/src/repository/list_metadata.rs` - Repository with all operations
37+
- `wp_mobile_cache/migrations/0007-create-list-metadata-tables.sql` - Schema
38+
- `wp_mobile/src/service/metadata.rs` - MetadataService implementation
39+
- `wp_mobile/src/service/posts.rs` - PostService integration
40+
41+
## Test Coverage
42+
43+
- `wp_mobile_cache`: 112 tests (31 new for list_metadata)
44+
- `wp_mobile`: 60 tests (15 new for MetadataService)
45+
46+
---
47+
48+
## Stale State on App Launch ✅ RESOLVED
49+
50+
### Problem
51+
52+
The `ListState` enum includes transient states (`FetchingFirstPage`, `FetchingNextPage`) that should not persist across app launches. If the app crashes during a fetch, these states remain in the database, causing perpetual loading indicators or blocked fetches on next launch.
53+
54+
### Solution Implemented
55+
56+
**Option B: Reset on `WpApiCache` initialization** was chosen.
57+
58+
After `perform_migrations()` completes, we reset all fetching states to `Idle`:
59+
60+
```rust
61+
// In WpApiCache::perform_migrations()
62+
Self::reset_stale_fetching_states_internal(connection);
63+
```
64+
65+
### Why Option B Over Option A
66+
67+
Option A (reset in `MetadataService::new()`) was rejected because `MetadataService` is not a singleton. Multiple services (PostService, CommentService, etc.) each create their own `MetadataService` instance. Resetting on each instantiation would incorrectly reset states when a new service is created mid-session.
68+
69+
`WpApiCache` is typically created once at app startup, making it the right timing for session-boundary cleanup.
70+
71+
### Design Decisions
72+
73+
- **`Error` state is NOT reset**: It represents a completed (failed) operation, not an in-progress one. Preserving it allows UI to show "last sync failed" and aids debugging.
74+
- **Logs when states are reset**: Helps debugging by printing count of reset states.
75+
76+
### Theoretical Issues (Documented in Code)
77+
78+
If an app architecture creates multiple `WpApiCache` instances during a session (e.g., recreating after user logout/login), this would reset in-progress fetches. In practice this is rare, but the documentation in `WpApiCache::reset_stale_fetching_states_internal` explains alternatives if needed.
79+
80+
See full documentation in `wp_mobile_cache/src/lib.rs`.
81+
82+
---
83+
84+
## Remaining Work
85+
86+
See `metadata_service_implementation_plan.md` for full details:
87+
88+
- **Phase 3.3**: Update collection creation to use sync callback
89+
- **Phase 3.4**: Remove deprecated in-memory store (after migration)
90+
- **Phase 4**: Observer split (data vs state observers)
91+
- **Phase 5.3**: Update example app

wp_mobile_cache/src/lib.rs

Lines changed: 90 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -262,7 +262,13 @@ impl WpApiCache {
262262
pub fn perform_migrations(&self) -> Result<i64, SqliteDbError> {
263263
self.execute(|connection| {
264264
let mut mgr = MigrationManager::new(connection)?;
265-
mgr.perform_migrations().map_err(SqliteDbError::from)
265+
let version = mgr.perform_migrations().map_err(SqliteDbError::from)?;
266+
267+
// Reset stale fetching states after migrations complete.
268+
// See `reset_stale_fetching_states` for rationale.
269+
Self::reset_stale_fetching_states_internal(connection);
270+
271+
Ok(version)
266272
})
267273
}
268274

@@ -301,6 +307,89 @@ impl WpApiCache {
301307
}
302308

303309
impl WpApiCache {
310+
/// Resets stale fetching states (`FetchingFirstPage`, `FetchingNextPage`) to `Idle`.
311+
///
312+
/// # Why This Is Needed
313+
///
314+
/// The `ListState` enum includes transient states that represent in-progress operations:
315+
/// - `FetchingFirstPage` - A refresh/pull-to-refresh is in progress
316+
/// - `FetchingNextPage` - A "load more" pagination fetch is in progress
317+
///
318+
/// If the app is killed, crashes, or the process terminates while a fetch is in progress,
319+
/// these states will persist in the database. On the next app launch:
320+
/// - UI might show a perpetual loading indicator
321+
/// - New fetch attempts might be blocked if code checks "already fetching"
322+
/// - State is inconsistent with reality (no fetch is actually in progress)
323+
///
324+
/// # Why We Reset in `WpApiCache` Initialization
325+
///
326+
/// We considered several approaches:
327+
///
328+
/// 1. **Reset in `MetadataService::new()`** - Rejected because `MetadataService` is not
329+
/// a singleton. Multiple services (PostService, CommentService, etc.) each create
330+
/// their own `MetadataService` instance. Resetting on each instantiation would
331+
/// incorrectly reset states when a new service is created mid-session.
332+
///
333+
/// 2. **Reset in `WpApiCache` initialization** (this approach) - Chosen because
334+
/// `WpApiCache` is typically created once at app startup, making it the right
335+
/// timing for session-boundary cleanup.
336+
///
337+
/// 3. **Session tokens** - More complex: tag states with a session ID and treat
338+
/// mismatched sessions as stale. Adds schema complexity for minimal benefit.
339+
///
340+
/// 4. **In-memory only for fetching states** - Keep transient states in memory,
341+
/// only persist `Idle`/`Error`. Adds complexity in state management.
342+
///
343+
/// # Theoretical Issues
344+
///
345+
/// This approach assumes `WpApiCache` is created once per app session. If an app
346+
/// architecture creates multiple `WpApiCache` instances during a session (e.g.,
347+
/// recreating it after a user logs out and back in), this would reset in-progress
348+
/// fetches. In practice:
349+
/// - Most apps create `WpApiCache` once at startup
350+
/// - If your architecture differs, consider wrapping this in a "first launch" check
351+
/// or using a session token approach
352+
///
353+
/// # Note on `Error` State
354+
///
355+
/// We intentionally do NOT reset `Error` states. These represent completed (failed)
356+
/// operations, not in-progress ones. Preserving them allows:
357+
/// - UI to show "last sync failed" on launch
358+
/// - Debugging by inspecting `error_message`
359+
///
360+
/// If you need a fresh start, the user can trigger a refresh which will overwrite
361+
/// the error state.
362+
fn reset_stale_fetching_states_internal(connection: &mut Connection) {
363+
use crate::list_metadata::ListState;
364+
365+
// Reset both fetching states to idle
366+
let result = connection.execute(
367+
"UPDATE list_metadata_state SET state = ?1 WHERE state IN (?2, ?3)",
368+
params![
369+
ListState::Idle.as_db_str(),
370+
ListState::FetchingFirstPage.as_db_str(),
371+
ListState::FetchingNextPage.as_db_str(),
372+
],
373+
);
374+
375+
match result {
376+
Ok(count) if count > 0 => {
377+
eprintln!(
378+
"WpApiCache: Reset {} stale fetching state(s) from previous session",
379+
count
380+
);
381+
}
382+
Ok(_) => {
383+
// No stale states found - normal case
384+
}
385+
Err(e) => {
386+
// Log but don't fail - table might not exist yet on fresh install
387+
// (though we run this after migrations, so it should exist)
388+
eprintln!("WpApiCache: Failed to reset stale fetching states: {}", e);
389+
}
390+
}
391+
}
392+
304393
/// Execute a database operation with scoped access to the connection.
305394
///
306395
/// This is the **only** way to access the database. The provided closure

0 commit comments

Comments
 (0)