Repository navigation
test(datastore): recognise COMMIT after a statement close in the retry-safe fault proxy - #4927
Conversation
…y-safe fault proxy The fault proxy decided whether a client write was the COMMIT by looking only at the first packet of each TCP read. The driver writes COM_STMT_CLOSE right before COMMIT, so a single read could hold both packets. The COMMIT was then missed, no fault was injected and Save returned nil, which made the test flaky on every MySQL image and deterministic on mariadb:11.8. isCommitPacket now walks every packet in the buffer, skipping empty packets and ignoring a partial packet at the end of the read. A table-driven test pins the packet walk.
…yte packet lengths The retry-safe fault proxy test built its COM_STMT_CLOSE packet from bare numbers and only used payloads short enough for a one-byte length. Name the command byte and statement id length, and add rows with payloads that need the second and third length bytes before the COMMIT.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Packet scanning and validation internal/datastore/v2only/save_retry_safe_mysql_test.go |
The helper scans complete packets for COMMIT queries. Tests cover standalone and concatenated packets, multi-byte payload lengths, non-COMMIT packets, and incomplete input. |
Priority:
Estimated code review effort: 2 (Simple) | ~8 minutes
Change: Other
Suggested labels: pkg:datastore
Merge Risk: 
616be
-
The remaining split-read edge case does not establish a new merge-blocking risk for this test-only change.
Pre-merge checks: 
-
Check name Status Explanation Resolution Description check The description explains the failure, root cause, implementation, test coverage, and results. However, all checklist items remain unchecked, including the required relicensing agreement. Update the checklist to reflect completed work. Check the required relicensing agreement only after confirming acceptance, and check the contributing-guidelines, test, lint, documentation, and feature-PR items as applicable. Docstring Coverage Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly and specifically identifies the main change: updating the retry-safe fault proxy test to recognize COMMIT after a statement close. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 1 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Code Review ✅ Approved🟢 Low risk · The test-only proxy now detects COMMIT after preceding packets, fixing fault-injection coverage without changing application behavior. Fixes the retry-safe fault proxy so VerificationRules No rules evaluated Functional validation Not enabledFor more verification details, comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
Description
TestSaveDetection_RetrySafeMarkerMatchesCommitOutcome_MySQLfailed on every run onmariadb:11.8and intermittently on the other server legs, which turned thedatabase-tests (mariadb-11.8)job red onmain. It is a test bug, not a product bug.The test sits a fault-injecting TCP proxy between the app and the server and detects the
COMMITof the save transaction to drop it or to lose its reply. The detector looked only at the first packet of each TCP read. The MySQL driver writes the close of a prepared statement right before theCOMMIT, so one read can hold both packets, theCOMMITis then not recognised, no fault is injected, andSavereturns nil where the test expects an error. Whether the two packets arrive in one read or two is a scheduling race, so the test passed only by chance.isCommitPacketnow walks every packet in the read and matches theCOMMITin any of them, skipping empty packets and ignoring a partial trailing packet. A table-driven test coversCOMMITalone, after a statement close, after an empty packet, lower case, a non-COMMITquery, empty, short and partial buffers, and payloads past one and two length bytes. The product code is unchanged; when the fault does fire the existing assertions hold on every server.Tests: the save test passes 5 of 5 runs on
mariadb:11.8,mariadb:10.11andmysql:8.4, and 3 of 3 onmariadb:11.4andmysql:9.7.4; before the change it failed 3 or 4 of 4 runs on the first four.golangci-lintwith theintegrationtag reports 0 issues.Related issue
None.
Checklist
task testand/ornpm test)task lintand/ornpm run check:all)Licensing (required)
BirdNET-Go is moving from CC BY-NC-SA 4.0 to an OSI-approved open source license (AGPL-3.0). See the tracking issue #4243. Contributions cannot be merged until the author agrees to this, because contributors keep copyright and the current inbound license does not grant relicensing rights.
Summary by CodeRabbit