Rework error handling #3549
Rework error handling #3549
Conversation
| /// - database: `Database` object | ||
| /// - results: `Joined<Package, Repository>` results to update | ||
| /// - stage: Processing stage | ||
| func updatePackages(client: Client, |
There was a problem hiding this comment.
updatePackages (note the plural) was only called from analysis. Moving this into the Analyze namespace (which I'll update to Analysis at some point in the future) makes this explicit.
| try await recordError(database: database, error: error, stage: stage) | ||
| static func updatePackage(client: Client, | ||
| database: Database, | ||
| result: Result<Joined<Package, Repository>, Error>) async throws { |
There was a problem hiding this comment.
We temporarily introduce specific static updatePackage functions on Analysis (and further down Ingestion) to deal with the fact that Ingestion has transitioned over to a new error type which we're throwing in typed throws while analysis is still using untyped throws and any Error as its error type.
The plan is to consolidate the two again once analysis moves to typed throws as well and base them both on the shared protocol ProcessingError.
| .unwrap() | ||
| XCTAssertEqual(repo.licenseUrl, "license") | ||
| for pkg in try await Package.query(on: app.db).all() { | ||
| XCTAssertEqual(pkg.processingStage, .ingestion, "\(pkg.url) must be in ingestion") |
There was a problem hiding this comment.
This is a crucial bit of additional test coverage where we ensure the processing stage is advanced when there are errors.
| @@ -354,12 +364,12 @@ class IngestorTests: AppTestCase { | |||
|
|
|||
| func test_ingest_unique_owner_name_violation() async throws { | |||
| // Test error behaviour when two packages resolving to the same owner/name are ingested: | |||
| // - don't update package | |||
| // - don't create repository records | |||
There was a problem hiding this comment.
This is an important change to how we're dealing with this error scenario. Previously, we did two things:
- avoid creating a duplicate repository record
- avoid updating the related package
We could preserve this behaviour but it actually simplifies things quite a bit to drop the second part of this. And it's the theme of this change: make sure processing is advanced through a stages despite errors.
In order to ensure this change doesn't break things in the following stage, this test now explicitly runs analysis as part of its validation to ensure it doesn't raise any errors (see below).
| try await Analyze.analyze(client: app.client, database: db, mode: .id(.id0)) | ||
| try await Analyze.analyze(client: app.client, database: db, mode: .id(.id1)) | ||
| try await XCTAssertEqualAsync(try await Package.find(.id0, on: db)?.processingStage, .analysis) | ||
| try await XCTAssertEqualAsync(try await Package.find(.id1, on: db)?.processingStage, .analysis) |
There was a problem hiding this comment.
This is where we're ensuring analysis can deal with this scenario where one package doesn't have a corresponding repository record.
|
I'll ad hoc deploy this to |
| case repositorySaveFailed | ||
| case repositorySaveUniqueViolation | ||
| } | ||
| var shouldFail: @Sendable (_ failureMode: FailureMode) -> Bool = { _ in false } |
There was a problem hiding this comment.
I've added this error triggering mechanism, which allows us to configure an environment variable FAILURE_MODE as for example
{
"fetchMetadataFailed": 0.1,
"findOrCreateRepositoryFailed": 0,
"invalidURL": 0,
"noRepositoryMetadata": 0,
"repositorySaveFailed": 0,
"repositorySaveUniqueViolation": 0
}which will trigger a fetchMetadataFailed error at a rate of 10%.
I'm using this to ensure on dev that there's no knock-on effect from errors in subsequent stages. It's also made it easier to ensure locally that the error logs are ok.
We can revert this after testing.
finestructure
force-pushed
the
rework-error-handling
branch
from
6c5b960 to
cf31064
Compare
|
Ok, this is all working as expected: 
|
finestructure
force-pushed
the
rework-error-handling
branch
4 times, most recently
from
6f837a5 to
e857fe1
Compare
|
There's no release of swift-concurrency-extras yet but after switching to track the main branch we got rid of the package name conflict warning at least. We could go ahead and merge this and drop the extra dependency clause once there's been a release in a follow-up. |
# Conflicts: # Sources/App/Commands/Ingest.swift
# Conflicts: # Sources/App/Core/Extensions/S3Store+ext.swift
finestructure
force-pushed
the
rework-error-handling
branch
from
e857fe1 to
475ce9d
Compare
|
Now that we've got 1.3.1 of swift-concurrency-extras I've rebased all the package manifest changes out of the PR. |
Rationale: https://noteplan.co/n/575ADC0D-8D6C-4ADF-BB25-972717C42C41
To do
test_ingest_continue_on_error)devto confirm things work as expectedThis is best reviewed with the "ignore whitespace" setting to hide all the indentation noise from new namespaces.