Skip to content

Break actor on errors that rollback transactions - #3832

Merged
kentonv merged 1 commit into
mainfrom
shrima/STOR-3841
Apr 22, 2025
Merged

kentonv merged 1 commit into
mainfrom
shrima/STOR-3841

Conversation

@shrima-cf

Copy link
Copy Markdown
Contributor

We should break output gates and reset actors in the event that we catch the following exceptions because they implicitly roll back any transaction:

SQLITE_FULL: database or disk full
SQLITE_IOERR: disk I/O error
SQLITE_BUSY: database in use by another process
SQLITE_NOMEM: out of memory
https://www.sqlite.org/lang_transaction.html#response_to_errors_within_a_transaction

@shrima-cf

Copy link
Copy Markdown
Contributor Author

See #3832 for context

Comment thread src/workerd/io/actor-sqlite.c++
Comment thread src/workerd/util/sqlite-test.c++ Outdated
Comment thread src/workerd/util/sqlite-test.c++ Outdated
Comment thread src/workerd/util/sqlite-test.c++ Outdated
Comment thread src/workerd/util/sqlite-test.c++ Outdated
Comment thread src/workerd/util/sqlite-test.c++ Outdated
Comment thread src/workerd/util/sqlite-test.c++ Outdated
Comment thread src/workerd/util/sqlite.c++
Comment thread src/workerd/util/sqlite.c++
Comment thread src/workerd/util/sqlite.c++ Outdated
@shrima-cf
shrima-cf requested a review from a team April 9, 2025 21:43
@shrima-cf
shrima-cf requested a review from a team as a code owner April 9, 2025 21:43
@github-actions

github-actions Bot commented Apr 9, 2025 •

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@shrima-cf
shrima-cf force-pushed the shrima/STOR-3841 branch 3 times, most recently from 4309615 to 23db545 Compare April 10, 2025 04:08
@shrima-cf
shrima-cf requested a review from kentonv April 10, 2025 04:11

@kentonv kentonv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, at this point I just have straightforward comments. Yay!

But, what about an end-to-end test? I don't see any here nor in the internal repo. Can we test this somehow? Maybe using an SRS testDirective to inject an error?

Comment thread src/workerd/util/sqlite-test.c++ Outdated
Comment thread src/workerd/util/sqlite-test.c++ Outdated
Comment thread src/workerd/util/sqlite-test.c++ Outdated
Comment thread src/workerd/util/sqlite.c++ Outdated
Comment thread src/workerd/util/sqlite.c++ Outdated
Comment thread src/workerd/util/sqlite.c++ Outdated
Comment thread src/workerd/util/sqlite.h Outdated
Comment thread src/workerd/util/sqlite.h Outdated
Comment thread src/workerd/util/sqlite.h Outdated
Comment thread src/workerd/util/sqlite-test.c++ Outdated
Comment thread src/workerd/util/sqlite-test.c++ Outdated
@kentonv

kentonv commented Apr 17, 2025

Copy link
Copy Markdown
Member

LGTM, just needs the fixups squashed and the internal test.

@shrima-cf
shrima-cf force-pushed the shrima/STOR-3841 branch 2 times, most recently from 5a611d4 to ce5670c Compare April 17, 2025 22:24
Comment thread src/workerd/api/sql-test.js Outdated
@shrima-cf
shrima-cf force-pushed the shrima/STOR-3841 branch 2 times, most recently from 4d18cba to d9466ba Compare April 21, 2025 17:43
@github-actions

github-actions Bot commented Apr 21, 2025 •

Copy link
Copy Markdown

The generated output of @cloudflare/workers-types matches the snapshot in types/generated-snapshot 🎉

Comment thread src/workerd/api/sql-test.js
@shrima-cf
shrima-cf force-pushed the shrima/STOR-3841 branch 2 times, most recently from 112390b to 20082fd Compare April 21, 2025 19:12
@shrima-cf

Copy link
Copy Markdown
Contributor Author

@cloudflare/wrangler Could someone help review this PR so I can merge it?

@kentonv
kentonv merged commit 8a44fc0 into main Apr 22, 2025
@kentonv
kentonv deleted the shrima/STOR-3841 branch April 22, 2025 02:33
@kentonv

kentonv commented Apr 22, 2025

Copy link
Copy Markdown
Member

I bypassed codeowner check and merged.

@danlapid why are our merges being blocked on review from the wrangler team?

@danlapid

Copy link
Copy Markdown
Collaborator

I bypassed codeowner check and merged.

@danlapid why are our merges being blocked on review from the wrangler team?

As a follow up to an incident of D1 a few weeks ago we set wrangler as a required CODEOWNER on types changes.

The ANT team has historically been the one to maintain and own the types generation and the generated types and they have a better grasp of the possible implications of those changes.

They are usually very quick to review but I suspect that due to Friday and Monday being bank holidays in the UK this pull request didn't get much attention.

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.

3 participants