execution_server: pre-validate CAS blobs and return PreconditionFailure

[amankrx]

Description

Inline definition of google.rpc.PreconditionFailure. Bazel's RemoteSpawnRunner inspects this proto in the gRPC Status.details of a FAILED_PRECONDITION response and, when violations of type MISSING are present, automatically re-uploads the named blobs and retries the Execute call. Without this detail Bazel surfaces the failure as a hard build error — exactly the symptom triggered by an interrupted previous build leaving partial CAS uploads.

Fixes # (issue)

Type of change

Please delete options that aren't relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to
  not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Please also list any relevant details for your test configuration

Checklist

• Updated documentation if needed
• Tests added/amended
• bazel test //... passes locally
• PR is contained in a single commit, using git amend see some docs

---

This change is 

[MarcusSorealheis]

Please do not do use string-parsing errors in action_messages.rs.**

extract_missing_blob_digest is fragile in a way that will silently rot. You're grepping for "not found in either fast or slow store" and then for "Object " … " not found", all of which are formatting choices owned by a different crate. Nothing prevents someone from rewording that message. There's no compile-time link, no test in the producing crate that pins the format. The day someone tweaks the fast-slow store's error string to be more helpful, the FAILED_PRECONDITION path silently degrades to a plain NotFound, Bazel stops auto-retrying, and you'll find out from a customer.

The right fix is structured, not textual. Attach the missing digest as typed metadata on the Error (an enum variant, an explicit field, or even a sentinel Code) at the point it's generated, and have to_execute_response match on that. The pre-check in execution_server.rs already does this properly. It knows the digest because it asked. The asymmetry between the two paths is the tell that the worker path took the shortcut. I'd block on this; the rest of the patch is solid enough that this one piece sticks out.

If you're not going to fix it now, at least export the marker as a pub const from wherever it's produced, and have that crate's tests assert the format. Don't let two unrelated files agree on a string by accident.

PreconditionFailure is defined four times. Once in execution_server.rs, once in action_messages.rs, and once in each of the two test files. The "avoid pulling tonic-types" justification is thin.tonic-types is small, and even if you don't want it, one pub(crate) definition in nativelink-util beats four. The current state means future schema changes (unlikely, but) require four edits and four chances to drift. Pick one home.

The nested Result<Result<Stream, Status>, Error> in inner_execute is hard to read. Two error channels in one return type isn't intuitive. At the call site you immediately unwrap the outer with ? and then match the inner. Either a small enum ExecuteOutcome { Stream(_), Reject(Status) } or a helper that returns Result<Stream, Status> and lifts internal errors to Status::internal would read better. Minor, but the file already has enough going on.

The Redis fix is the right call, but the match is awkward. This is cleaner:

let create_result = result.or_else(|e| {
    if e.kind() == redis::ErrorKind::Extension
        && e.code() == Some("Index")
        && e.detail() == Some("already exists")
    {
        Ok(())
    } else {
        Err(e).err_tip(|| format!("Error with ft_create in …"))
    }
});

Also, how confident are you that code()/detail() parsing is stable across RediSearch versions? "Index already exists" is the current text but I'd grep the RediSearch source to confirm capitalization hasn't drifted between 2.x branches. A case-insensitive contains("already exists") is uglier but probably safer.

The pre-check is racy with eviction between has_many and worker fetch — fine, the worker path covers it, but say so in the comment so the next reader doesn't re-litigate it. prost_types::Any is used in execution_server.rs and bare Any in action_messages.rs; pick one. The pre-check is intentionally shallow (top-level only, no nested Directory walk). The comment in action_messages.rs mentions this but execution_server.rs should too, since that's where someone looking to extend the check will land first.

The comments explaining why (the RemoteSpawnRunner behavior, the REv2 subject format) are the right level of detail and will save the next person a lot of time. So nice work there.

All in all, solid PR but just need to fix a few joints before we can merge er.