From b8c3ca92f29f1bf690c42e7f1fb02cbb704dc792 Mon Sep 17 00:00:00 2001 From: Lahfir Date: Tue, 30 Jun 2026 23:02:57 -0700 Subject: [PATCH] fix(session): resolve Greptile review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - status honesty: TraceConfig::has_sink() (feeds status "tracing") returns false once the writer has failed, so status no longer reports active tracing while emits are being dropped. - gc data race: re-check recent activity (snapshot/trace mtime, created-recently) AFTER acquiring the session write lock, so a trace segment written between the pre-check and remove is not deleted. - session end honors the resolved session: `--session X session end` (no id) now ends X, not whatever the current_session pointer names. - degrade precision: manifest/pointer reads degrade only symlinked/corrupt files (the structural attack) but let genuine I/O/permission errors surface, instead of masking every failure as "absent". - strict batch override: a batch item overriding to a valid no-trace/ended session no longer fails INVALID_ARGS under a --trace-strict parent (clone_with_session_segment(None) yields a sinkless config, not a strict error). Kept: BatchSessionArgs deny_unknown_fields (session batch args are new — no legacy generators to break — and it catches the no_trce typo the prior review required). Guards added: failed-writer-reports-no-sink, strict-parent-allows-no-trace- batch-override. --- crates/core/src/context_tests.rs | 22 ++++++++++++++++++++++ crates/core/src/session/gc.rs | 32 +++++++++++++++++++------------- crates/core/src/session/mod.rs | 26 +++++++++++++------------- crates/core/src/trace.rs | 16 ++++++++++++++-- crates/core/src/trace_tests.rs | 12 ++++++++++++ src/dispatch/mod.rs | 4 +++- 6 files changed, 83 insertions(+), 29 deletions(-) diff --git a/crates/core/src/context_tests.rs b/crates/core/src/context_tests.rs index 80e15ba..eea1927 100644 --- a/crates/core/src/context_tests.rs +++ b/crates/core/src/context_tests.rs @@ -321,3 +321,25 @@ fn batch_item_session_override_uses_its_own_segment_dir() { .trace_dir(); assert!(child_trace.is_dir()); } + +#[test] +fn strict_parent_allows_no_trace_batch_override() { + let _guard = crate::refs_test_support::HomeGuard::new(); + let traced = start_session(StartSessionOptions { + name: None, + trace: SessionTraceMode::On, + force: true, + }) + .unwrap(); + let untraced = start_session(StartSessionOptions { + name: None, + trace: SessionTraceMode::Off, + force: true, + }) + .unwrap(); + let parent = CommandContext::new(Some(traced.id.clone()), None, true).unwrap(); + let child = parent + .for_batch_item(Some(untraced.id.clone())) + .expect("a no-trace session override must not fail under a strict parent"); + assert_eq!(child.session_id(), Some(untraced.id.as_str())); +} diff --git a/crates/core/src/session/gc.rs b/crates/core/src/session/gc.rs index c60c0ca..ad861dc 100644 --- a/crates/core/src/session/gc.rs +++ b/crates/core/src/session/gc.rs @@ -1,6 +1,6 @@ use super::{ - TRACE_LIVENESS_WINDOW, list_sessions, now_millis, read_current_session_pointer, read_manifest, - session_dir, + SessionManifest, TRACE_LIVENESS_WINDOW, list_sessions, now_millis, + read_current_session_pointer, read_manifest, }; use crate::{ context::validate_session_id, @@ -31,22 +31,23 @@ pub fn pointer_references_live_session() -> Result { pub fn is_live(session_id: &str) -> Result { validate_session_id(session_id)?; let store = RefStore::for_session(Some(session_id))?; - let base = store.base_dir(); - if lock_holder_is_live(&base.join("refstore.lock")) { + if lock_holder_is_live(&store.base_dir().join("refstore.lock")) { return Ok(true); } - if path_recently_modified(&base.join("snapshots")) + let manifest = read_manifest(session_id)?; + Ok(has_recent_activity(&store, manifest.as_ref())) +} + +fn has_recent_activity(store: &RefStore, manifest: Option<&SessionManifest>) -> bool { + if path_recently_modified(&store.base_dir().join("snapshots")) || trace_dir_recently_written(&store.trace_dir()) { - return Ok(true); + return true; } - if let Some(manifest) = read_manifest(session_id)? { + manifest.is_some_and(|manifest| { let age = Duration::from_millis(now_millis().saturating_sub(manifest.created_at)); - if manifest.ended_at.is_none() && age < TRACE_LIVENESS_WINDOW { - return Ok(true); - } - } - Ok(false) + manifest.ended_at.is_none() && age < TRACE_LIVENESS_WINDOW + }) } pub fn gc(options: GcOptions) -> Result { @@ -71,11 +72,16 @@ pub fn gc(options: GcOptions) -> Result { } else if manifest.ended_at.is_none() { continue; } - let dir = session_dir(&manifest.id)?; + let store = RefStore::for_session(Some(&manifest.id))?; + let dir = store.base_dir().to_path_buf(); let lock = match RefStoreLock::acquire(&dir.join("refstore.lock")) { Ok(lock) => lock, Err(_) => continue, }; + if has_recent_activity(&store, Some(&manifest)) { + drop(lock); + continue; + } let did_remove = remove_session_dir(&dir)?; drop(lock); if did_remove { diff --git a/crates/core/src/session/mod.rs b/crates/core/src/session/mod.rs index 5b74158..05fcba2 100644 --- a/crates/core/src/session/mod.rs +++ b/crates/core/src/session/mod.rs @@ -71,22 +71,17 @@ pub fn read_current_session_pointer() -> Result, AppError> { let mut file = match open_session_file(&path) { Ok(file) => file, Err(err) if err.kind() == ErrorKind::NotFound => return Ok(None), - Err(err) => { + Err(err) if is_symlinked(&path) => { tracing::warn!( - "ignoring unreadable session pointer {}: {err}", + "ignoring symlinked session pointer {}: {err}", path.display() ); return Ok(None); } + Err(err) => return Err(err.into()), }; let mut id = String::new(); - if let Err(err) = file.read_to_string(&mut id) { - tracing::warn!( - "ignoring unreadable session pointer {}: {err}", - path.display() - ); - return Ok(None); - } + file.read_to_string(&mut id)?; let id = id.trim().to_string(); if id.is_empty() || validate_session_id(&id).is_err() { return Ok(None); @@ -112,18 +107,23 @@ pub fn read_manifest(session_id: &str) -> Result, AppErr let mut file = match open_session_file(&path) { Ok(file) => file, Err(err) if err.kind() == ErrorKind::NotFound => return Ok(None), - Err(err) => return Ok(ignore_unreadable_manifest(&path, &err)), + Err(err) if is_symlinked(&path) => return Ok(ignore_unreadable_manifest(&path, &err)), + Err(err) => return Err(err.into()), }; let mut json = String::new(); - if let Err(err) = file.read_to_string(&mut json) { - return Ok(ignore_unreadable_manifest(&path, &err)); - } + file.read_to_string(&mut json)?; match serde_json::from_str(&json) { Ok(manifest) => Ok(Some(manifest)), Err(err) => Ok(ignore_unreadable_manifest(&path, &err)), } } +fn is_symlinked(path: &Path) -> bool { + std::fs::symlink_metadata(path) + .map(|meta| meta.file_type().is_symlink()) + .unwrap_or(false) +} + fn ignore_unreadable_manifest( path: &Path, err: &E, diff --git a/crates/core/src/trace.rs b/crates/core/src/trace.rs index a9001fb..0892009 100644 --- a/crates/core/src/trace.rs +++ b/crates/core/src/trace.rs @@ -144,7 +144,13 @@ impl TraceConfig { } pub(crate) fn has_sink(&self) -> bool { - !matches!(self.state.pending, TracePending::None) + if matches!(self.state.pending, TracePending::None) { + return false; + } + match self.state.writer.lock() { + Ok(writer) => !matches!(*writer, WriterState::Failed), + Err(_) => true, + } } pub(crate) fn pending_file_path(&self) -> Option<&Path> { @@ -161,7 +167,13 @@ impl TraceConfig { if self.pending_file_path().is_some() { return Ok(self.clone()); } - Self::build(None, session_segment_dir, self.strict) + match session_segment_dir { + Some(dir) => Self::build(None, Some(dir), self.strict), + None => Ok(Self { + strict: self.strict, + state: Arc::new(TraceState::default()), + }), + } } } diff --git a/crates/core/src/trace_tests.rs b/crates/core/src/trace_tests.rs index d9ef9f7..2668cc4 100644 --- a/crates/core/src/trace_tests.rs +++ b/crates/core/src/trace_tests.rs @@ -273,3 +273,15 @@ fn segment_open_rejects_symlinked_trace_dir() { let _ = fs::remove_file(&base); let _ = fs::remove_dir_all(&real); } + +#[test] +fn failed_writer_reports_no_sink() { + let missing = std::env::temp_dir() + .join("agent-desktop-nodir-hassink") + .join("trace.jsonl"); + let config = TraceConfig::build(Some(missing), None, false).unwrap(); + assert!( + !config.has_sink(), + "a trace writer whose open failed must not report an active sink" + ); +} diff --git a/src/dispatch/mod.rs b/src/dispatch/mod.rs index 01f8d52..304f157 100644 --- a/src/dispatch/mod.rs +++ b/src/dispatch/mod.rs @@ -372,7 +372,9 @@ pub(crate) fn dispatch( no_trace: s.no_trace, force: s.force, }), - SessionAction::End(e) => session::execute(session::SessionAction::End { id: e.id }), + SessionAction::End(e) => session::execute(session::SessionAction::End { + id: e.id.or_else(|| context.session_id().map(str::to_string)), + }), SessionAction::List => session::execute(session::SessionAction::List), SessionAction::Gc(g) => session::execute(session::SessionAction::Gc { older_than_secs: g.older_than,