Connections lack a "being deleted" state (Unknown error occurred after deletion single connection and back to the Onboarding page)

[SofiiaZaitseva]

Environment

• OS Version / Instance: macOS
• Deployment: Cloud
• Step where error happened: Onboarding

Steps to Reproduce

1. Create the first connection from the onboarding page
2. Go to Connection settings
3. Delete existing connection
4. Go back to Onboarding

Current Behavior

Unknown error occurred

Expected Behavior

The user should be navigated to the third step of connection creation (it should be confirmed)

Logs

FinalStep.tsx:40 Uncaught TypeError: Cannot read properties of undefined (reading 'connectionId')
at FinalStep (FinalStep.tsx:40:1)
at renderWithHooks (react-dom.development.js:14985:1)
at mountIndeterminateComponent (react-dom.development.js:17811:1)
at beginWork (react-dom.development.js:19049:1)
at HTMLUnknownElement.callCallback (react-dom.development.js:3945:1)
at Object.invokeGuardedCallbackDev (react-dom.development.js:3994:1)
at invokeGuardedCallback (react-dom.development.js:4056:1)
at beginWork$1 (react-dom.development.js:23964:1)
at performUnitOfWork (react-dom.development.js:22776:1)
at workLoopSync (react-dom.development.js:22707:1)

[octavia-squidington-iii]

cc @airbytehq/frontend

[dizel852]

After some time of debugging, I've found the root cause.
Good news - it's actually not a UI issue 😅

The problem is in the consistent state of the connection(s).
After deleting the connection, it's still observable via some endpoints, for example:
We deleted the connection via POST /api/v1/connections/delete but after a few seconds we still can get it in a list by sending the POST /api/v1/web_backend/connections/list request
That's why UI crashes, it gets the response where {"hasConnections":true} while it's actually false(in process of deletion ) 🤷🏻‍♂️

Here is a recorded example:
https://www.loom.com/share/0d80016539134de585375afcab1315a1

@timroes whom should we ping to make the 'connection deletion' operation consistent? I guess we need to create a new issue assigned to the Backend team. WDYT?

[timroes]

@evantahler @benmoriceau Could the OSS team have a look at that? It seems those the /web_backend/connections/list API and the /v1/web_backend/workspace/state are still seeing a deleted connection for some time and only after some time no longer returning it. This is causing some issues in the frontend.

[evantahler]

Yep, that's a bug!

I think that showing a connection in the process of being deleted is /web_backend/connections/list might be the correct behavior - in @dizel852's Loom video, there is still a running sync to finish and that takes some time. I could see that information still being valuable to users, who may want to watch that final sync's logs, for example.

I do agree that /v1/web_backend/workspace/state either needs to be modified to ignore deleted connections, or should have an additional property. If this API was changed to return counts rather than booleans, you could compute what to display on the front end:

// Option A
{
  hasConnections: false, // <-- Changed to false; `hasConnections` only counts non-deleted connections
  hasSources: true,
  hasDestinations: true,
}

// Option B
{
  connectionsCount: 1, 
  deletedConnectionsCount: 1,
  sourcesCount: 1,
  destinationsCount: 1,
}

const showOnboarding: boolean = !!!(connectionsCount - deletedConnectionsCount)

Which option do you like?

[timroes]

I think I'd prefer keeping the booleans for ease of use.

…

On Wed, Aug 3, 2022, 17:42 Evan Tahler ***@***.***> wrote: Yep, that's a bug! I think that showing a connection in the process of being deleted is /web_backend/connections/list might be the correct behavior - in @dizel852 <https://github.com/dizel852>'s Loom video, there is still a running sync to finish and that takes some time. I could see that information still being valuable to users, who may want to watch that final sync's logs, for example. I do agree that /v1/web_backend/workspace/state either needs to be modified to ignore deleted connections, or should have an additional property. If this API was changed to return counts rather than booleans, you could compute what to display on the front end: // Option A{ hasConnections: false, // <-- Changed to false; `hasConnections` only counts non-deleted connections hasSources: true, hasDestinations: true,} // Option B{ connectionsCount: 1, deletedConnectionsCount: 1 sourcesCount: 1, destinationsCount: 1,} const showOnboarding: boolean = !!!(connectionsCount - deletedConnectionsCount) Which option do you like? — Reply to this email directly, view it on GitHub <#15137 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGWFLML74CHG4WYY266UKDVXKHPPANCNFSM55ALQMJQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[dizel852]

I think I'd prefer keeping the booleans for ease of use.
…
On Wed, Aug 3, 2022, 17:42 Evan Tahler @.> wrote: Yep, that's a bug! I think that showing a connection in the process of being deleted is /web_backend/connections/list might be the correct behavior - in @dizel852 https://github.com/dizel852's Loom video, there is still a running sync to finish and that takes some time. I could see that information still being valuable to users, who may want to watch that final sync's logs, for example. I do agree that /v1/web_backend/workspace/state either needs to be modified to ignore deleted connections, or should have an additional property. If this API was changed to return counts rather than booleans, you could compute what to display on the front end: // Option A{ hasConnections: false, // <-- Changed to false; hasConnections only counts non-deleted connections hasSources: true, hasDestinations: true,} // Option B{ connectionsCount: 1, deletedConnectionsCount: 1 sourcesCount: 1, destinationsCount: 1,} const showOnboarding: boolean = !!!(connectionsCount - deletedConnectionsCount) Which option do you like? — Reply to this email directly, view it on GitHub <#15137 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGWFLML74CHG4WYY266UKDVXKHPPANCNFSM55ALQMJQ . You are receiving this because you were mentioned.Message ID: @.>

Agree - also prefer to keep the booleans, since there is no need for a counter, at least for now 🤷🏻‍♂️

[evantahler]

I did some research on this, and this is a bigger issue than I had initially thought:

• In the database, we only have 3 statuses for a connection: active, inactive, depreciated
• The way a connection is deleted is though a temporal activity, by ending the connectionManager for that connection... whose last step is changing the status column for the connection to depreciated
• Because there is only one lifecycleManager instance for each connection, the request to delete the connection queues up behind any running syncs. So, as you've seen, a running sync remains in the active status until the current sync concludes. The only place that the request for a connection to be deleted's information resides is within temporal, not the database, which isn't great.

So, to fix up this API, there are probably 3 paths forward:

1. Add a new column to the database pendingDelete(bool)
2. Add a new option to the status Enum connections.status like pendingDelete
3. Allow the ConfigRepository.countConnectionsForWorkspace method to both query the DB and then Temporal to double check that the connections found are not about to be deleted.

I think #3 is the worst idea because it turns a simple count() query into something that needs to inspect all connections and talk to the temporal API - making it slow and adding complexity. We call these APIs a lot.

I think #1 is the best idea because "pendingDelete" is metadata about the connection, and isn't really a state (the activity to delete the connection may fail), and it keeps these APIs only querying the database.

[davinchia]

Driveby - I agree #1 seems to be the best solution here given my understanding of the system.

[gosusnp]

@evantahler, I am currently looking at a similar change for Versioning.

My take between options 1 and 2 is that option 2 was probably less error prone.
By adding a new column, any consumer of status=active need to filter out the pendingDelete=true rows.
One alternative might be to update both, set status to inactive and pendingDelete to true to preserve behavior of the query for active connections.

[evantahler]

Removing myself as the assigned person from this issue.

I had started work on this in (#15422) but it raised a larger question - why do we store connection status in temporal /before/ persisting it to the database? Why do all state changes for connection objects go though temporal? This is one of the many issues that led to the creation of the Thoughts on improving Temporal at Airbyte doc which proposes relying less on Temporal's

cc @salima-airbyte @jdpgrailsdev - I think the team should consider a more holistic approach to "drift between the DB and Temporal", and not the hack I had originally started in the earlier PR.

[edmundito]

@timroes Since we no longer have onboarding, would you know if this issue is still relevant?

[davinchia]

Closing since we no longer have onboarding. Please reopen if I am wrong!