More reliably abort commands if client disconnects.#2635
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12d3496f43
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce53637b5d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
tylerkaraszewski
changed the title
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
danieldoglas
left a comment
danieldoglas
left a comment
There was a problem hiding this comment.
LGTM.
Should we try to set this after acquiring the lock in SQLite::prepare? That way, even if we acquired the lock after waiting 60s, we can abort without commiting if the client disconnected.
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
So I think this is a good idea, and thought about adding it, but I think I'm OK omitting or at least deferring it. It requires passing the command or a pointer to the But I think what I'd like more is to adjust this to be interruptible so that instead of potentially waiting 20 seconds for the lock and then doing nothing with it, we could at least interrupt waiting for the lock when the client disconnects. This is also a little bigger change. What if we do this as a followup? |
|
@danieldoglas - updated, comments addressed. |
|
@codex review |
I agree with this. Let's address in a followup then! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 387844dc5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| command->_inDBWriteOperation = true; | ||
| command->process(_db); | ||
| command->_inDBWriteOperation = false; | ||
| _throwIfAborted(command); |
There was a problem hiding this comment.
When a client disconnects after this new post-process() check passes, but before the worker/sync commit path actually commits the transaction, shouldAbort can still be set by handleSocket while the command is waiting for the commit lock; I checked the worker and sync commit sections in BedrockServer.cpp, and they only call getRemainingTime()/commit() after processCommand() returns NEEDS_COMMIT, with no abort check or abort ref. In that timing window the write is still committed and only the reply send fails, so long commit-lock waits or slow commits can still persist changes for a disconnected client despite this change's rollback-on-disconnect behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is the case we are addressing in a followup, I think it's fine.
|
Followup issue: https://github.com/Expensify/Expensify/issues/648226 |
danieldoglas
left a comment
danieldoglas
left a comment
There was a problem hiding this comment.
LGTM, just one question. Not sure if we need to discuss that here or in the slack.
| @@ -140,7 +146,7 @@ void BedrockCommand::_waitForHTTPSRequests() | |||
| requestIt++; | |||
| } | |||
|
|
|||
| // Timed everything out, can return. | |||
| // Timed everything out (or abandoned them), can return. | |||
There was a problem hiding this comment.
While I think these changes make sense, I'm kind of conflicted with aborting the command if we did HTTP requests. This could be a problem for cases like when we do billing, right?
There was a problem hiding this comment.
I know what you mean but there's no real guarantee these succeed anyway. They can time out and fail after an HTTPS request anyway, or we can send a HTTPS request and get disconnected from the remote server while receiving the response. Billing is designed to be resilient to failures anyway, so if we start abandoning commands mid-billing, we could re-run it, but it doesn't actually affect normal billing because those commands are fire-and-forget and are exempt to being cancelled.
2 participants
Details
This extends our Abort for commands where the client has disconnected from just inside sql queries to before and after each command phase, and during HTTPS requests.
Fixed Issues
Fixes https://expensify.slack.com/archives/C0B96MS9B8X/p1781194408074179
Tests
Internal Testing Reminder: when changing bedrock, please compile auth against your new changes