Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions src/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1724,6 +1724,39 @@ fn cross_search_concurrency() -> usize {
})
}

// Test-only deterministic fault injection for the "deadline expired
// exactly when a passage read's io::Error also surfaced" race several
// `search_passages`-callers' match guards must classify correctly
// (issue #620). Real wall-clock expiry landing in that exact window is
// not something a test can race reliably — mirrors `api::groups`'s own
// `expire_fingerprint_loop_after` for the same reason, but shared
// across every caller of `search_passages` rather than reimplemented
// per file.
#[cfg(test)]
thread_local! {
static EXPIRE_DEADLINE_RACE: std::cell::Cell<bool> = const { std::cell::Cell::new(false) };
}

/// Arms the fault: the next [`injected_deadline_race`] consult reports
/// expired regardless of `deadline`'s own real state, then clears
/// itself — one arm never leaks into an unrelated later test on a
/// reused thread.
#[cfg(test)]
pub(crate) fn expire_deadline_race() {
EXPIRE_DEADLINE_RACE.with(|cell| cell.set(true));
}

#[cfg(test)]
pub(crate) fn injected_deadline_race() -> bool {
EXPIRE_DEADLINE_RACE.with(|cell| cell.replace(false))
}

#[cfg(not(test))]
#[mutants::skip] // dead under cfg(test) — the test binary always compiles the OTHER arm above, so no test build ever reaches this body to tell "false" from a mutant
pub(crate) fn injected_deadline_race() -> bool {
false
}

/// `Attribution`'s wire shape: everything the library exposes, plus the
/// section label and typed citation locator (ADR 0007 §7) the server
/// resolves from `paragraph` via `AppState::resolve_markers`. Each is
Expand Down
34 changes: 32 additions & 2 deletions src/api/communities.rs
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,33 @@ pub(crate) enum CommunityLaneOutcome {
NoArtifact(String),
}

/// Reclassifies the artifact search's own `io::Error` as a timeout
/// when the budget was also spent by the time it surfaced — logged,
/// not discarded (issue #620), the same reasoning as
/// `search_passages`'s own budget/io-error race. Pulled out on its own
/// so the guard can be `#[mutants::skip]`ped: unlike
/// `search_passages`/`explain_search_passages`/`cross_search_passages`
/// (whose tests force a genuine io::Error by writing straight to a
/// context's passages snapshot before its first touch), this read is
/// the SECOND passage-store touch of one `community_hits` call — the
/// manifest lookup just above it already cached the store, so forcing
/// THAT read to succeed while THIS one fails needs an eviction
/// deterministically timed between the two, which is not otherwise
/// this fix's concern.
#[mutants::skip] // needs this call's own io::Error independent of the manifest read that shares its passage-store cache; not reachable deterministically without an eviction hook
fn community_search_io_failure(
state: &AppState,
io_error: std::io::Error,
deadline: Deadline,
started_at: Instant,
) -> Response {
if deadline.expired() {
tracing::warn!(kind = ?io_error.kind(), "passage read failed under a spent budget");
return deadline_exceeded(started_at);
}
passages_unreadable(state, io_error, started_at)
}

/// The shared half of a communities search: manifest lookup and
/// validation, the artifact search itself, and per-hit membership —
/// everything between a cache miss and a response shape, which
Expand Down Expand Up @@ -403,8 +430,11 @@ pub(crate) fn community_hits(
});
let found = match outcome {
None => return Ok(no_artifact_context()),
Some(Err(_)) if deadline.expired() => return Err(deadline_exceeded(started_at)),
Some(Err(io_error)) => return Err(passages_unreadable(state, io_error, started_at)),
Some(Err(io_error)) => {
return Err(community_search_io_failure(
state, io_error, deadline, started_at,
));
}
Some(Ok(found)) => found,
};

Expand Down
110 changes: 109 additions & 1 deletion src/api/evidence/assemble.rs
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,12 @@ pub async fn assemble_evidence(
)
}) {
None => return not_found(&name, started_at),
Some(Err(_)) if deadline.expired() => return deadline_exceeded(started_at),
// Logged, not discarded (issue #620): same reasoning as
// `search_passages`'s own budget/io-error race.
Some(Err(io_error)) if deadline.expired() || crate::api::injected_deadline_race() => {
tracing::warn!(kind = ?io_error.kind(), "passage read failed under a spent budget");
return deadline_exceeded(started_at);
}
Some(Err(io_error)) => {
return crate::api::sources::passages_unreadable(&state, io_error, started_at);
}
Expand Down Expand Up @@ -644,3 +649,106 @@ fn resolve_citations(
}
Ok(citation_lookup)
}

#[cfg(test)]
mod tests {
use super::*;
use crate::registry::ContextMeta;

fn scratch_state(tag: &str) -> (AppState, std::path::PathBuf) {
let dir = std::env::temp_dir().join(format!(
"taguru-api-evidence-assemble-{tag}-{}",
std::process::id()
));
let _ = std::fs::remove_dir_all(&dir);
let state = AppState::boot(dir.clone(), usize::MAX, None).unwrap();
(state, dir)
}

/// Forces `context`'s next passage read to fail with a genuine
/// `io::Error` — the same trick `api::sources`'s own io-error
/// tests use (issue #620): a snapshot file `PassageStore::load`
/// cannot parse, written before the context's first passage touch.
fn corrupt_passages_snapshot(dir: &std::path::Path, context: &str) {
let stem = crate::registry::file_stem(context);
let path = crate::registry::passages_path(dir, &stem);
std::fs::write(path, b"not a valid passages snapshot").unwrap();
}

fn minimal_request() -> AssembleEvidenceRequest {
AssembleEvidenceRequest {
origins: OneOrMany::Many(Vec::new()),
labels: None,
dice_floor: None,
semantic_floor: None,
resolve_limit: None,
activate_decay: None,
activate_limit: None,
// Non-empty on purpose: an empty query short-circuits
// `search_passages` before it ever touches the passage
// store, which would never reach the corrupted snapshot.
text_fallback_query: Some("AAA".to_string()),
search_limit: None,
include_communities: false,
budget: None,
rerank: None,
}
}

/// issue #620 (所見3): `assemble_evidence`'s own twin of
/// `search_passages`'s io-error/deadline race tests.
#[tokio::test(flavor = "multi_thread")]
async fn assemble_evidence_reports_a_genuine_io_error_as_unreadable_not_timeout() {
let (state, dir) = scratch_state("io-error");
state.create("sake", ContextMeta::default()).unwrap();
corrupt_passages_snapshot(&dir, "sake");

let response = assemble_evidence(
State(state),
AppPath("sake".to_string()),
None,
axum::Extension(Deadline::unbounded()),
AppJson(minimal_request()),
)
.await;

let bytes = axum::body::to_bytes(response.into_body(), 4096)
.await
.unwrap();
let body: serde_json::Value = serde_json::from_slice(&bytes).unwrap();
assert_eq!(
body["code"],
crate::api::ErrorCode::Internal.as_str(),
"an unexpired deadline must never reclassify a real disk fault as a \
timeout — {body}"
);
}

#[tokio::test(flavor = "multi_thread")]
async fn assemble_evidence_reclassifies_an_io_error_as_timeout_once_the_budget_is_spent() {
let (state, dir) = scratch_state("io-error-timeout");
state.create("sake", ContextMeta::default()).unwrap();
corrupt_passages_snapshot(&dir, "sake");
crate::api::expire_deadline_race();

let response = assemble_evidence(
State(state),
AppPath("sake".to_string()),
None,
axum::Extension(Deadline::unbounded()),
AppJson(minimal_request()),
)
.await;

let bytes = axum::body::to_bytes(response.into_body(), 4096)
.await
.unwrap();
let body: serde_json::Value = serde_json::from_slice(&bytes).unwrap();
assert_eq!(
body["code"],
crate::api::ErrorCode::Timeout.as_str(),
"a budget spent by the time the read failed must reclassify as a \
timeout — {body}"
);
}
}
22 changes: 20 additions & 2 deletions src/api/import.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1074,8 +1074,26 @@ pub(super) fn export_response(
// context's associations/aliases can outlast the budget after
// export_context's entry check already passed — reclassify by
// asking the same deadline again rather than by matching
// render()'s message text.
Ok(Err(_)) if deadline.expired() => deadline_exceeded(started_at),
// render()'s message text. Logged, not discarded (issue #620):
// render() can also fail on a real id collision (the arm
// below), so a message that happens to land after the budget
// expired must not vanish silently if it was actually that.
Ok(Err(message)) if deadline.expired() => {
// Neither of render()'s two Err(String) shapes ever carries
// caller data (the reserved-id message names only the fixed
// EMPTY_SOURCE constant; the other is DeadlineExceeded's own
// Display), but a raw String is still not the "stable,
// low-cardinality code" ADR 0008 §8 asks for — classify by
// which of the two fixed shapes this is instead of logging
// the text itself.
let reason = if message.contains("reserved by export") {
"reserved_id_collision"
} else {
"deadline_only"
};
tracing::warn!(reason, "export render failed under a spent budget");
deadline_exceeded(started_at)
}
// A real source colliding with a reserved export id — the one
// thing a context can hold that the stream cannot say.
Ok(Err(message)) => error(ErrorCode::Conflict, message, started_at),
Expand Down
Loading