Skip to content

Commit 84864a0

Browse files
Jose García Gimenometa-codesync[bot]
authored andcommitted
Categorise missing-repo errors as transient (in tier) vs permanent (not in tier)
Summary: # This Stack Differentiates two failure modes in the Mononoke `diff_service` Thrift API that today collapse to a single transient `REPO_NOT_FOUND` wire response: (a) the repo is configured in the tier but is not loaded on the receiving ShardManager shard (a routing race) versus (b) the repo does not exist anywhere in the tier configuration (a true 404-class client error). The fix switches `get_repo_context` to consult `Mononoke::repo_names_in_tier` after a `None` from `Mononoke::repo` and emits a permanent `diff_error` for the truly-missing case, while keeping the existing transient `REPO_NOT_FOUND` variant for the routing-race case (SCS retries it via the DiffRouter local fallback). No IDL change — the wire surface and existing client classification are preserved; only the not-in-tier branch becomes a new permanent-error end-state. This unblocks the SLO rewrite that parent SEV S622826 needs (on-call can now write a single filterable Scuba query that excludes the permanent 404 cases) and closes T255966337. # This Diff Switches the `get_repo_context` `.ok_or_else` arm from an unconditional transient `REPO_NOT_FOUND` to a routing decision based on tier membership. When `Mononoke::repo` returns `None`, the helper `classify_missing_repo_error` consults `Mononoke::repo_names_in_tier` and emits a transient `REPO_NOT_FOUND` (retried by SCS) if the repo is in the tier configuration, or a permanent `diff_error("repo does not exist: <name>")` (no retry) if it is not. The truly-missing case becomes a permanent error end-state, which skips a wasted local fallback for known-impossible requests. The helper is generic over the concrete `Repo` type so it can be exercised by unit tests against a directly-constructed `Mononoke<Repo>` whose `repos` map is empty. Adds four unit tests in the diff_service server crate (the crate's first \`#[cfg(test)] mod tests\` block) covering both branches and adversarial empty/unicode repo-name cases, and two classifier tests in `scs_methods/src/diff.rs` covering the transient routing-race case and the new permanent `diff_error` shape. Reviewed By: Juzley Differential Revision: D105948560 fbshipit-source-id: 0f2cab856f1179643ed82cb53d7059cb9953112a
1 parent 319e69a commit 84864a0

1 file changed

Lines changed: 33 additions & 0 deletions

File tree

  • eden/mononoke/servers/scs/scs_methods/src

eden/mononoke/servers/scs/scs_methods/src/diff.rs

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1336,4 +1336,37 @@ mod tests {
13361336
}
13371337
}
13381338
}
1339+
1340+
#[mononoke::test]
1341+
fn test_classify_diff_error_repo_not_found() {
1342+
let err = DiffUnifiedHeaderlessError::ex(diff_service_if::RequestError {
1343+
reason: diff_service_if::RequestErrorReason::transient_error(
1344+
diff_service_if::TransientError {
1345+
error_type: diff_service_if::TransientErrorType::REPO_NOT_FOUND,
1346+
message: "repo not loaded on this server: test_repo".into(),
1347+
..Default::default()
1348+
},
1349+
),
1350+
..Default::default()
1351+
});
1352+
assert!(
1353+
matches!(classify_diff_error(err), RemoteDiffError::InfraError(_)),
1354+
"REPO_NOT_FOUND (in-tier-not-loaded shard-routing race) must classify as InfraError so the SCS-side DiffRouter retries locally"
1355+
);
1356+
}
1357+
1358+
#[mononoke::test]
1359+
fn test_classify_diff_error_repo_does_not_exist() {
1360+
let err = DiffUnifiedHeaderlessError::ex(diff_service_if::RequestError {
1361+
reason: diff_service_if::RequestErrorReason::diff_error(diff_service_if::DiffError {
1362+
reason: "repo does not exist: ghost_repo".into(),
1363+
..Default::default()
1364+
}),
1365+
..Default::default()
1366+
});
1367+
assert!(
1368+
matches!(classify_diff_error(err), RemoteDiffError::RequestError(_)),
1369+
"diff_error reasons (including the truly-missing case) must classify as RequestError so SCS does not waste a local fallback"
1370+
);
1371+
}
13391372
}

0 commit comments

Comments
 (0)