Skip to content

Conversation

@HappyZombies
Copy link
Contributor

This PR removes the CLIENT_DEPRECATE_EOF client capability (stored as DEPRECATE_EOF in code), which is currently advertised but not supported and can cause protocol errors. It also adds a regression test to ensure only valid client capability flags are applied and unknown flags are ignored.

This PR resolves #4009, view the item for more details and the investigation done for this.

@codecov
Copy link

codecov bot commented Jan 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.87%. Comparing base (06db470) to head (c89aba6).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4033      +/-   ##
==========================================
- Coverage   89.87%   89.87%   -0.01%     
==========================================
  Files          86       86              
  Lines       13625    13624       -1     
  Branches     1618     1619       +1     
==========================================
- Hits        12245    12244       -1     
  Misses       1380     1380              
Flag Coverage Δ
compression-0 88.98% <ø> (-0.01%) ⬇️
compression-1 89.84% <ø> (-0.01%) ⬇️
static-parser-0 87.44% <ø> (-0.01%) ⬇️
static-parser-1 88.21% <ø> (-0.01%) ⬇️
tls-0 89.27% <ø> (-0.01%) ⬇️
tls-1 89.64% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@wellwelwel
Copy link
Collaborator

wellwelwel commented Jan 25, 2026

Hey, @HappyZombies! I can't see the need to include a test that checks for a non-existent variable, since it has been permanently removed. I think a test for this scenario would be a connection that fails when using the DEPRECATE_EOF flag, and when removing it, the test ends successfully.

For example, "capturing" this log from #4009:

MySQL Protocol - response ERROR
  Packet Length: 51
  Packet Number: 0
  Response Code: ERR Packet (0xff)
  Error Code: 1158
  SQL state: 08S01
  Error message: Got an error reading communication packets

Then, testing the same connection + query without the flag and this time, it should pass.

What do you think?

@HappyZombies
Copy link
Contributor Author

HappyZombies commented Jan 25, 2026

@wellwelwel I'm good with that change, however this means I would need to leave the constant value of DEPRECATE_EOF in order to have test that checks for a failure on on/off, since the only way to do a test like that after removing the constant would be to manually modify the 'config.clientFlags', or other work arounds.

I’m okay doing that if we think it’s the more valuable test, just wanted to call out that it would rely on directly modifying internal config rather than using a supported option. In which case I can just do this test since the current one isn't that valuable

Now of course unless you think it's beneficial to leave that non working constant of DEPRECATE_EOF flag ...

@wellwelwel
Copy link
Collaborator

Now of course unless you think it's beneficial to leave that non working constant of DEPRECATE_EOF flag

I would suggest we remove the test/integration/connection/test-flags.test.cjs test before merging. The reproduction test I mentioned would be more beneficial for the issue itself ($4009), as proof of concept.

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.

Setting connection flag of "DEPRECATE_EOF" causes MySQL Server v8.0.44 to throw an error?

2 participants