fix: close_reading_session_v2 takes timestamp, sets closed_at_ms

Review finding #3: close now accepts timestamp_ms and sets closed_at_ms
automatically, removing the need for a separate set_session_closed_at_ms call.
set_session_closed_at_ms retained for override use cases.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
wangdl 2026-06-11 21:33:42 +08:00
parent 1a0095cd58
commit 8ab767ee4a
4 changed files with 36 additions and 30 deletions

View File

@ -283,7 +283,7 @@ mod tests {
} }
fn teardown_session(id: &str) { fn teardown_session(id: &str) {
let _ = session_v2::close_reading_session_v2(id); let _ = session_v2::close_reading_session_v2(id, 0);
let _ = session_v2::remove_session_v2(id); let _ = session_v2::remove_session_v2(id);
} }
@ -398,7 +398,7 @@ mod tests {
fn test_closed_session_rejects() { fn test_closed_session_rejects() {
let _guard = lock_test(); let _guard = lock_test();
let sid = setup_session("mat_reject"); let sid = setup_session("mat_reject");
session_v2::close_reading_session_v2(&sid).unwrap(); session_v2::close_reading_session_v2(&sid, 3000).unwrap();
assert!(push_heartbeat_v2(&sid, "mat_reject", 15, None, 3000).is_err()); assert!(push_heartbeat_v2(&sid, "mat_reject", 15, None, 3000).is_err());
teardown_session(&sid); teardown_session(&sid);
} }

View File

@ -106,13 +106,14 @@ pub fn resume_reading_session_v2(session_id: &str) -> Result<(), SessionError> {
Ok(()) Ok(())
} }
/// Close the session. No more events allowed. /// Close the session and record its closed_at_ms. No more events allowed.
pub fn close_reading_session_v2(session_id: &str) -> Result<(), SessionError> { pub fn close_reading_session_v2(session_id: &str, timestamp_ms: i64) -> Result<(), SessionError> {
with_session_mut(session_id, |s| { with_session_mut(session_id, |s| {
if s.status == ReadingSessionStatus::Closed { if s.status == ReadingSessionStatus::Closed {
return Err(SessionError::AlreadyClosed); return Err(SessionError::AlreadyClosed);
} }
s.status = ReadingSessionStatus::Closed; s.status = ReadingSessionStatus::Closed;
s.closed_at_ms = Some(timestamp_ms);
Ok(()) Ok(())
}) })
} }
@ -311,7 +312,7 @@ mod tests {
} }
fn teardown(id: &str) { fn teardown(id: &str) {
let _ = close_reading_session_v2(id); let _ = close_reading_session_v2(id, 0);
let _ = remove_session_v2(id); let _ = remove_session_v2(id);
} }
@ -333,7 +334,7 @@ mod tests {
assert_eq!(get_session_v2(&id).unwrap().status, ReadingSessionStatus::Paused); assert_eq!(get_session_v2(&id).unwrap().status, ReadingSessionStatus::Paused);
resume_reading_session_v2(&id).unwrap(); resume_reading_session_v2(&id).unwrap();
assert_eq!(get_session_v2(&id).unwrap().status, ReadingSessionStatus::Active); assert_eq!(get_session_v2(&id).unwrap().status, ReadingSessionStatus::Active);
close_reading_session_v2(&id).unwrap(); close_reading_session_v2(&id, 0).unwrap();
assert_eq!(get_session_v2(&id).unwrap().status, ReadingSessionStatus::Closed); assert_eq!(get_session_v2(&id).unwrap().status, ReadingSessionStatus::Closed);
remove_session_v2(&id).unwrap(); remove_session_v2(&id).unwrap();
} }
@ -358,9 +359,9 @@ mod tests {
#[test] #[test]
fn test_closed_rejects_events() { fn test_closed_rejects_events() {
let id = start_reading_session_v2(test_material(), 1000).unwrap(); let id = start_reading_session_v2(test_material(), 1000).unwrap();
close_reading_session_v2(&id).unwrap(); close_reading_session_v2(&id, 0).unwrap();
assert!(record_session_event_v2(&id, 2000, 10, None).is_err()); assert!(record_session_event_v2(&id, 2000, 10, None).is_err());
assert!(close_reading_session_v2(&id).is_err()); assert!(close_reading_session_v2(&id, 0).is_err());
remove_session_v2(&id).unwrap(); remove_session_v2(&id).unwrap();
} }
@ -379,7 +380,7 @@ mod tests {
#[test] #[test]
fn test_not_found() { fn test_not_found() {
assert!(get_session_v2("nonexistent").is_err()); assert!(get_session_v2("nonexistent").is_err());
assert!(close_reading_session_v2("nonexistent").is_err()); assert!(close_reading_session_v2("nonexistent", 0).is_err());
} }
#[test] #[test]
@ -506,22 +507,31 @@ mod tests {
#[test] #[test]
fn test_closed_rejects_interrupted() { fn test_closed_rejects_interrupted() {
let id = start_reading_session_v2(test_material(), 1000).unwrap(); let id = start_reading_session_v2(test_material(), 1000).unwrap();
close_reading_session_v2(&id).unwrap(); close_reading_session_v2(&id, 0).unwrap();
assert!(mark_session_interrupted_v2(&id).is_err()); assert!(mark_session_interrupted_v2(&id).is_err());
assert!(mark_session_failed_v2(&id).is_err()); assert!(mark_session_failed_v2(&id).is_err());
remove_session_v2(&id).unwrap(); remove_session_v2(&id).unwrap();
} }
#[test] #[test]
fn test_set_closed_at_ms() { fn test_close_sets_closed_at_ms() {
let id = start_reading_session_v2(test_material(), 1000).unwrap(); let id = start_reading_session_v2(test_material(), 1000).unwrap();
assert!(get_session_v2(&id).unwrap().closed_at_ms.is_none()); assert!(get_session_v2(&id).unwrap().closed_at_ms.is_none());
close_reading_session_v2(&id).unwrap(); close_reading_session_v2(&id, 5000).unwrap();
set_session_closed_at_ms(&id, 5000).unwrap();
assert_eq!(get_session_v2(&id).unwrap().closed_at_ms, Some(5000)); assert_eq!(get_session_v2(&id).unwrap().closed_at_ms, Some(5000));
remove_session_v2(&id).unwrap(); remove_session_v2(&id).unwrap();
} }
#[test]
fn test_set_closed_at_ms_override() {
let id = start_reading_session_v2(test_material(), 1000).unwrap();
close_reading_session_v2(&id, 5000).unwrap();
// set_session_closed_at_ms can override if needed
set_session_closed_at_ms(&id, 9999).unwrap();
assert_eq!(get_session_v2(&id).unwrap().closed_at_ms, Some(9999));
remove_session_v2(&id).unwrap();
}
#[test] #[test]
fn test_get_active_session_by_material() { fn test_get_active_session_by_material() {
let mat_a = ReadingMaterialRefV2::new("mat_a", "/tmp/a.md", "a.md", "markdown"); let mat_a = ReadingMaterialRefV2::new("mat_a", "/tmp/a.md", "a.md", "markdown");
@ -535,7 +545,7 @@ mod tests {
assert_eq!(found.unwrap().client_session_id, id_a); assert_eq!(found.unwrap().client_session_id, id_a);
// Close session a, should not be found as active // Close session a, should not be found as active
close_reading_session_v2(&id_a).unwrap(); close_reading_session_v2(&id_a, 0).unwrap();
assert!(get_active_session_v2("mat_a").is_none()); assert!(get_active_session_v2("mat_a").is_none());
teardown(&id_a); teardown(&id_a);

View File

@ -41,8 +41,8 @@ fn resume_reading_session_v2(session_id: String) -> Result<(), String> {
} }
#[uniffi::export] #[uniffi::export]
fn close_reading_session_v2(session_id: String) -> Result<(), String> { fn close_reading_session_v2(session_id: String, timestamp_ms: i64) -> Result<(), String> {
zx_document_core::session_v2::close_reading_session_v2(&session_id).map_err(|e| e.to_string()) zx_document_core::session_v2::close_reading_session_v2(&session_id, timestamp_ms).map_err(|e| e.to_string())
} }
#[uniffi::export] #[uniffi::export]
@ -719,7 +719,7 @@ mod ffi_tests {
assert_eq!(e4.event_type, ReadingEventTypeV2::MarkedAsRead); assert_eq!(e4.event_type, ReadingEventTypeV2::MarkedAsRead);
push_material_closed_v2(sid.clone(), "mat_ffi_test".to_string(), 0, 12000).unwrap(); push_material_closed_v2(sid.clone(), "mat_ffi_test".to_string(), 0, 12000).unwrap();
close_reading_session_v2(sid.clone()).unwrap(); close_reading_session_v2(sid.clone(), 0).unwrap();
let exported = export_pending_events_v2(100, 13000); let exported = export_pending_events_v2(100, 13000);
assert!(exported.len() >= 4, "expected >=4, got {}", exported.len()); assert!(exported.len() >= 4, "expected >=4, got {}", exported.len());
@ -745,7 +745,7 @@ mod ffi_tests {
let sid = start_reading_session_v2(mat, 0).unwrap(); let sid = start_reading_session_v2(mat, 0).unwrap();
pause_reading_session_v2(sid.clone()).unwrap(); pause_reading_session_v2(sid.clone()).unwrap();
resume_reading_session_v2(sid.clone()).unwrap(); resume_reading_session_v2(sid.clone()).unwrap();
close_reading_session_v2(sid.clone()).unwrap(); close_reading_session_v2(sid.clone(), 0).unwrap();
let _ = zx_document_core::session_v2::remove_session_v2(&sid); let _ = zx_document_core::session_v2::remove_session_v2(&sid);
} }
@ -770,7 +770,7 @@ mod ffi_tests {
assert!(retry.iter().any(|ev| ev.event_id == e.event_id)); assert!(retry.iter().any(|ev| ev.event_id == e.event_id));
ack_events_v2(vec![e.event_id.clone()]); ack_events_v2(vec![e.event_id.clone()]);
close_reading_session_v2(sid.clone()).unwrap(); close_reading_session_v2(sid.clone(), 0).unwrap();
let _ = zx_document_core::session_v2::remove_session_v2(&sid); let _ = zx_document_core::session_v2::remove_session_v2(&sid);
} }
@ -871,6 +871,6 @@ mod ffi_tests {
#[test] #[test]
fn test_session_not_found() { fn test_session_not_found() {
assert!(close_reading_session_v2("nonexistent".to_string()).is_err()); assert!(close_reading_session_v2("nonexistent".to_string(), 0).is_err());
} }
} }

View File

@ -34,8 +34,7 @@ App 启动
退出阅读页 退出阅读页
→ push_material_closed_v2(sessionId, materialId, residualDelta, timestampMs) → push_material_closed_v2(sessionId, materialId, residualDelta, timestampMs)
→ close_reading_session_v2(sessionId) → close_reading_session_v2(sessionId, timestampMs)
→ set_session_closed_at_ms_v2(sessionId, timestampMs)
→ export_pending_events_v2(limit, timestampMs) → export_pending_events_v2(limit, timestampMs)
→ 每条 event 转为 ReadingEventUploadItem → 写入本地队列 → ack_events_v2(eventIds) → 每条 event 转为 ReadingEventUploadItem → 写入本地队列 → ack_events_v2(eventIds)
``` ```
@ -118,10 +117,8 @@ class V2ReadingRuntimeAdapter: ReadingRuntimeAdapter {
let ts = Int64(timestamp.timeIntervalSince1970 * 1000) let ts = Int64(timestamp.timeIntervalSince1970 * 1000)
// 1. Flush 事件 // 1. Flush 事件
try await flushPendingEvents(timestamp: timestamp) try await flushPendingEvents(timestamp: timestamp)
// 2. Close session // 2. Close session (sets closed_at_ms automatically)
try close_reading_session_v2(sessionId: sessionId) try close_reading_session_v2(sessionId: sessionId, timestampMs: ts)
// 3. Set closed time
try set_session_closed_at_ms_v2(sessionId: sessionId, timestampMs: ts)
} }
// ── Export + Queue + Ack ── // ── Export + Queue + Ack ──
@ -198,8 +195,10 @@ func safeCloseSession(sessionId: String?) {
os_log("Flush failed: %@", error.localizedDescription) os_log("Flush failed: %@", error.localizedDescription)
} }
let ts = Int64(Date().timeIntervalSince1970 * 1000)
do { do {
try close_reading_session_v2(sessionId: sid) try close_reading_session_v2(sessionId: sid, timestampMs: ts)
} catch let error as SessionError { } catch let error as SessionError {
if error == .alreadyClosed { if error == .alreadyClosed {
os_log("Session already closed, ignoring") os_log("Session already closed, ignoring")
@ -207,9 +206,6 @@ func safeCloseSession(sessionId: String?) {
} }
os_log("Close failed: %@", error.localizedDescription) os_log("Close failed: %@", error.localizedDescription)
} }
let ts = Int64(Date().timeIntervalSince1970 * 1000)
try? set_session_closed_at_ms_v2(sessionId: sid, timestampMs: ts)
} }
``` ```