Skip to content

[WIP] Add test coverage for error handling in database operations#959

Draft
Copilot wants to merge 1 commit into
mainfrom
copilot/test-error-handling-database-operations
Draft

[WIP] Add test coverage for error handling in database operations#959
Copilot wants to merge 1 commit into
mainfrom
copilot/test-error-handling-database-operations

Conversation

Copy link
Copy Markdown

Copilot AI commented May 13, 2026

Thanks for asking me to work on this. I will get started on it and keep this PR's description up to date as I form a plan and make progress.


This section details on the original issue you should resolve

<issue_title>Are errors handled correctly?</issue_title>
<issue_description>That could be a long road, but I'm not sure what will happen exactly when the database returns an error. Normally we expect all queries to return in time without errors and I think the case of the whole database is failing (e.g. shutdown/no network) is covered. But in theory, especially with implemented timeout logic, all cases in etherpad, where we access the database (no matter if it's a get or set) should have test coverage for their error callbacks.

We could have redo logic in some cases or disconnect the clients in other cases and for failing set operations we must ensure that the pad stays intact. The latter can be hard, as clients get ACCEPT_COMMIT from the read cache, while the value is not in the db yet. When a instance is shutdown (does it flush all values before exiting?) or restarted (clients are at revision X, reconnect logic kicks in and the server is at revision X minus some revisions - it's okay if the some of the last revisions are lost, but the clients should be able to reconnect gracefully).

I'm especially concerned of cases, where revisions are stored, but the atext is not yet stored or vice versa. (probably both set operations are included in the same bulk operation - but when we disable writeInterval and write values immediately to db, as soon as pooling is implemented, it could mean that one value is written, while the other is not.)

While I don't think it is very critical, it affects stability and I suspect most of the error handling is not covered during our tests.</issue_description>

Comments on the Issue (you are @copilot in this section)

@JohnMcLear Certainly something that needs to be address at some point :) Hopefully someone can pick it up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Are errors handled correctly?

2 participants