Repository navigation
fix: preserve sessions after client write resets - #444
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
ethanpailes
left a comment
ethanpailes
left a comment
There was a problem hiding this comment.
Sorry for the delayed review. I wrote it a while ago and forgot the actually submit it 🤦🏻
| /// leaves the peer open so the test also checks that shutdown wakes the input | ||
| /// thread, rather than relying on the client to close its own socket. | ||
| #[cfg(feature = "test_hooks")] | ||
| pub fn client_write_error(kind: &str) -> Option<std::io::Error> { |
There was a problem hiding this comment.
This only supports injecting a single write failure. We should build a more generic mechanism that lets you specify a set of failure points with a comma seperated list.
There was a problem hiding this comment.
Addressed in d17b95f. Failure points now support a comma-separated list, with each point consumed at most once per process. Added tests for concurrent consumption and builds with and without test hooks.
| /// MaybeSwitch writes share the heartbeat's client-disconnect failure mode. | ||
| #[test] | ||
| #[timeout(15000)] | ||
| fn maybe_switch_write_reset_preserves_session() -> anyhow::Result<()> { |
There was a problem hiding this comment.
It seems like this test covers all the codepaths that the heartbeat test does. Let's remove these wrappers and just unconditionally do the var tweak.
There was a problem hiding this comment.
Addressed in d17b95f. Removed the wrappers and consolidated the regression into a single test that unconditionally updates the variable. The test passes with the fix and fails with the original error handling.
|
I wanted to write a personal note by myself. I'm sorry for submitting such a heavily vibe-coded patch, and I really appreciate the time you've spent reviewing it. I understand that reviewing AI generated code can place an extra burden on maintainers, especially when the contributor has little experience with the language. To be honest, I really just want to do something forthe project, rather than simply issue a bug report. But my rust skills are quite limited, so I ended up relying almost entirely on Codex to write this patch. I realize this isn't ideal, and I apologize for any extra work it may have caused. I'll do my best to understand and address your feedback. Thank you again for your patience and for all the work you put into this project! |
| /// Consume a named failure point at most once per process. Configure points | ||
| /// with a comma-separated SHPOOL_TEST__FAILURE_POINTS environment variable. | ||
| #[cfg(feature = "test_hooks")] | ||
| pub fn fail_once(point: &str) -> bool { |
There was a problem hiding this comment.
Sorry for not saying this in the first round. Looking at this now, this seems a bit overfit to the task. Rather than just introducing the ability to introduce a failure on the very first call, I think we should have a maybe_fail hook that accepts some extra parameters.
What about this.: SHPOOL_TEST__FAILURE_POINTS can contain a comma seperated list of failure points, but whereas now they are just names, in this more flexible approach we will use the format <failure-point-name>:n_1:n_2:...:n_3, where n_1 are numbers indicating the number of calls to that particular failure point. Any number that is present will produce a failure, so heartbeat-write:2 would succeed the first time but fail the second and then succeeded for all calls thereafter. The call-site of maybe_fail would look the same as for this fail_once hook.
|
No worries about the AI generated patch. It's nicely focused and pretty easy to review. I know some projects have very agressive anti-AI policies, but for shpool, AI is allowed so long as you disclose it. AI can definitely produce bad interaction patterns in open source projects, but I think it can be a valuable tool if used appropriately. Automatic diagnosing of bugs that might otherwise require a bunch of back and forth with the maintainers which results in focused patches like this one seems like obviously a great use of the technology to me. I do really appreciate you adding human-authored text for your communications though! FYI: looks like this PR has fallen out of date with master and you'll need to rebase and fix some merge conflicts. |
Support comma-separated failure points with thread-safe, one-time consumption per process. Keep error construction at the call sites and disable injection without test_hooks. Consolidate the reset regression into one MaybeSwitch test with an unconditional variable update, as requested in review. Add coverage for multiple failure points and concurrent consumption with and without test_hooks. Validation: regression 11/11, var 7/7, template-switch 2/2, production cargo check, nightly rustfmt, and git diff --check passed. Clippy passed with the existing unwrap_or_default allowance. The merged regression rejects the original error handling.
d17b95f to
353364a
Compare
Issue Link
Related: #335 and #442 (unattachable sessions; the triggers discussed there differ).
AI Policy Ack
The AI Policy has been read and acknowledged.
This PR was mainly completed by GPT-6.1-Sol on Codex , including investigation, implementation, regression tests, and local validation. The explanation below is explicitly agent-generated.
This PR was:
Description
Agent-generated explanation — GPT-6.1-Sol on Codex: