fix(projects): Treat fetch failures as pending

[jjbayer]

When we fail to fetch a project state for any reason (max retries exceeded, redis error), we should not mark the project as invalid.

Currently we treat the project as if the DSN was invalid, and respond to clients with a 403 response. This might have made sense as a defensive measure in a pre-spooling world where envelopes could not be buffered for a longer period of time, but with spooling and time-based envelope eviction, we can now safely buffer them.

ref: INC-888

blocked by: https://github.com/getsentry/team-ingest/issues/345

[jjbayer]

With the old version, this call raises a 403 error.

[Dav1dde]

This does have the downside of potentially exhausting buffers if a project consistently cannot be computed. Do we need more alerts for this case?

[Dav1dde]

Possibly a bit nicer to read with:

    .inspect_err(|e| relay_log::error!(..))
    .unwrap_or(ProjectFetchState::pending())

[jjbayer]

I like how unwrap_or_else only creates a ProjectFetchState in the error case. ProjectFetchState::pending() calls Instant::now(), so it's not entirely free.

[loewenheim]

That part could be fixed by using unwrap_or_else, but I think the current version is fine as well.

[Dav1dde]

I went overboard with my example, I mainly meant the split of logging into the inspect_err, keep the side effect separate from the conversion. But either way it's fine and possibly worse with the how rustfmt wants to format it.

[jjbayer]

That part could be fixed by using unwrap_or_else

Yes, but then you need to ignore the argument passed to unwrap_or_else(the error), that you previously inspected with inspect_err. Is that better?

                .inspect_err(|e| {
                    relay_log::error!(
                        error = e as &dyn Error,
                        "Failed to fetch project from source"
                    );
                })
                .unwrap_or_else(|_| ProjectFetchState::pending());
                
                // vs
                
                .unwrap_or_else(|e| {
                    relay_log::error!(
                        error = &e as &dyn Error,
                        "Failed to fetch project from source"
                    );
                    ProjectFetchState::pending()
                });

[loewenheim]

No, I think the current version is superior.

[jjbayer]

This does have the downside of potentially exhausting buffers if a project consistently cannot be computed. Do we need more alerts for this case?

Good point, we should alert when the buffer gets close to its total capacity, or implement @iambriccardo 's automatic eviction policy. I added a task to the epic.

[loewenheim]

That part could be fixed by using unwrap_or_else, but I think the current version is fine as well.

[jjbayer]

Note: not merging this until https://github.com/getsentry/team-ingest/issues/345 is mature.