mysqlctl: upgrade pierrec/lz4 to v4 to fix broken amd64 block decoding - #20778
mysqlctl: upgrade pierrec/lz4 to v4 to fix broken amd64 block decoding#20778arthurschreiber wants to merge 4 commits into
Conversation
The v2 amd64 assembly decoder spills only the low byte of the register holding the LZ4 token across its call to runtime.memmove, so leftover upper bits can corrupt the next match length and fail an otherwise valid block with 'lz4: invalid source or destination buffer too short'. During a backup restore that error aborts the tablet, which is the recurring TestBackupMysqlctldWithlz4Compression CI failure: mysql.ibd is the only datadir file bigger than one 4 MiB lz4 block, and whether the leftover bits are non-zero depends on the memmove path the host CPU picks, which is why the failure comes and goes between runners. The v4 module decodes the same frames correctly (verified byte-for-byte against v2-written archives), so existing lz4 backups stay restorable. The writer moves from the v2 Header struct to v4 options; v4 takes a level enum instead of a raw hash-chain depth, so --compression-level values at or below 1 keep their old near-zero-search profile via the fast compressor and 2 through 9 map onto the matching hash-chain levels. The new multi-block round-trip test covers payloads larger than one lz4 block for every builtin engine; the failure itself is CPU-dependent, so no in-tree test can pin the decoder bug directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
Bot
added
NeedsWebsiteDocsUpdate
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20778 +/- ##
===========================================
- Coverage 69.67% 54.79% -14.89%
===========================================
Files 1614 46 -1568
Lines 216793 7492 -209301
===========================================
- Hits 151044 4105 -146939
+ Misses 65749 3387 -62362
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
arthurschreiber
added
Backport to: release-23.0
![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: ef438424de
ℹ️ 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".
| // decompressors through their multi-block paths, with long literal runs | ||
| // followed by matches inside every block; a round trip of a few bytes | ||
| // never leaves the first block. | ||
| func TestBuiltinCompressorsMultiBlock(t *testing.T) { |
There was a problem hiding this comment.
On hosts whose memmove path does not expose the v2 decoder bug, this same multi-block round trip succeeds against the parent dependency, so reverting the v4 upgrade still leaves the test green; it therefore adds four 9 MiB round trips without reliably guarding the fix. Either use a deterministic input/environment that fails with v2 or omit this purported regression test when no honest deterministic test is possible.
AGENTS.md reference: AGENTS.md:L79-L80
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The premise holds: on hosts whose memmove path leaves the token register's upper bits zero, this round trip passes with the v2 dependency too, and the PR description states that no deterministic in-tree test for the decoder bug is possible — the trigger is which runtime.memmove path the host CPU selects (golang/go#74925 documents the CPU dependence). The test is kept, with a different justification than guarding the decoder fix: multi-block payloads had no round-trip coverage at all (the existing test uses 14 bytes), this is the workload shape the backup engines actually produce, on FSRM-capable runners it does fail against v2, and it costs ~0.1s. The deterministic regression test in this PR is TestBuiltinCompressorsSequentialCopyN, added in ad4a403 after the reviewed commit, which pins the striped-copy failure that this PR's own CI surfaced.
| case level <= 1: | ||
| return lz4.Fast |
There was a problem hiding this comment.
When an existing deployment uses --compression-level=-1 with LZ4, the old writer passes that nonzero value to CompressBlockHC, where a nonpositive depth means an unlimited search; this level <= 1 branch instead silently selects the fast compressor. The same persisted configuration therefore changes from maximum hash-chain searching to the lowest-ratio mode after upgrade, so negative values should be mapped separately or rejected through the compatibility/deprecation process rather than grouped with 0 and 1.
AGENTS.md reference: AGENTS.md:L240-L244
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 809f521: the v2 writer ran CompressBlockHC for any non-zero level and block.go:269 treats a non-positive depth as winSize (65536), so a configured negative level meant the deepest search, not the fastest mode. Negative levels now map to Level9 (depth 131072, the closest the v4 enum offers), 0 and 1 stay on the fast compressor, and TestLz4CompressionLevelMapping pins the full mapping. The PR description and deployment notes describe the negative-level translation.
The v4 lz4 writer implements ReadFrom, but only for a writer with nothing written to it yet; io.Copy and io.CopyN select the destination's ReadFrom whenever the source has no visible WriteTo. The striped xtrabackup backup round-robins io.CopyN over its destination writers, so the second block for a stripe found the writer in writeState and the whole backup failed with 'lz4: unhandled state[writeState]'. The v4 reader's WriteTo carries the same fresh-stream restriction. Wrapper types now hide ReadFrom and WriteTo on the lz4 compressor and decompressor, so every copy goes through Write and Read regardless of how callers combine io.Copy and io.CopyN. The sequential-CopyN test pins the striped copy shape for all builtin engines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad4a4034c5
ℹ️ 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".
| } | ||
|
|
||
| type ( | ||
| // writeCloserOnly hides every method of the wrapped writer except |
There was a problem hiding this comment.
The wrapper documentation added here is explanatory commentary, which the repository instructions explicitly prohibit unless it was requested; keep the implementation limited to the requested change and remove these comments.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I'm keeping the comments: they state a constraint the code cannot express — io.Copy and io.CopyN select ReadFrom/WriteTo whenever those methods are visible, and the lz4 v4 implementations only accept a fresh stream — which is the only thing standing between a one-field wrapper type and a future simplification that reinstates the xb_backup CI failure. The rule against explanatory commentary targets narration of what code does; a comment recording why the wrapper must exist is the kind I want in this codebase.
| return decompressor, err | ||
| } | ||
|
|
||
| type ( |
There was a problem hiding this comment.
This introduces another mid-file type block even though the repository convention requires all declarations in one type (...) block at the top of the file; place these wrappers in that consolidated block rather than adding a separate declaration here.
AGENTS.md reference: AGENTS.md:L182-L186
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in e95eff9: the wrapper types now sit in a single type block at the top of the file. The pre-existing externalCompressor and externalDecompressor types stay mid-file with their methods; consolidating them is unrelated to this PR.
| CompressionLevel: compressionLevel, | ||
| lz4Writer := lz4.NewWriter(writer) | ||
| if err := lz4Writer.Apply( | ||
| lz4.ConcurrencyOption(backupCompressBlocks), |
There was a problem hiding this comment.
When an existing deployment sets --backup-storage-number-blocks=0 or a negative value, the v2 writer accepted it (0 selected serial compression and a negative value selected GOMAXPROCS), whereas v4's ConcurrencyOption rejects values below 1. Passing the flag through directly therefore makes Apply fail and aborts every LZ4 backup at compressor creation after upgrade; normalize these legacy values before applying the option.
AGENTS.md reference: AGENTS.md:L240-L244
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The stated failure does not occur: ConcurrencyOption in v4.1.27 normalizes any non-positive value to runtime.GOMAXPROCS(0) at options.go:131-134 and never rejects it, so Apply cannot fail on the flag value and no backup aborts. The adjacent real change is the opposite of a rejection: v2's WithConcurrency treated 0 (and 1) as serial compression and only negatives as GOMAXPROCS, so a configured --backup-storage-number-blocks=0 would have silently jumped from serial to all-cores compression. Fixed in e95eff9: 0 now maps to 1, which the v4 writer treats as serial, and negative values pass through with the same GOMAXPROCS meaning in both versions. TestLz4ConcurrencyBlocksMapping pins the mapping.
…evel The lz4 v2 writer ran the hash-chain compressor for any non-zero level and treated a non-positive search depth as unlimited, so a configured --compression-level=-1 meant the slowest, best-ratio mode; grouping negative values with 0 and 1 flipped them to the fastest mode. A negative level now maps to Level9, whose search depth of 131072 is the closest the v4 enum offers to the old unlimited search. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
The lz4 v2 writer compressed serially for a --backup-storage-number-blocks value of 0, while the v4 concurrency option reads any non-positive value as GOMAXPROCS; 0 now maps to 1, which the v4 writer treats as serial, so the configured behavior is unchanged. Negative values mean GOMAXPROCS in both versions and pass through. The stream-wrapper types move into a single type block at the top of the file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
Description
The recurring
Cluster (21)CI failure inTestBackupMysqlctldWithlz4Compression("process 'vttablet' exited prematurely") turned out to be a real product bug, not a flaky test: restoring an lz4 backup fails withlz4: invalid source or destination buffer too shortonmysql.ibd, both restore attempts fail identically, and the tablet exits.The bug is in the vendored
pierrec/lz4v2 amd64 assembly decoder. In itsmemmove_litpath it spills only the low byte of the register holding the LZ4 token across theCALL runtime·memmove, and restores only that byte. Whatevermemmoveleaves in the upper 56 bits then flows into the next sequence's match length, and an in-bounds match can fail the destination bounds check — the decoder rejects a perfectly valid block. Whethermemmoveleaves those bits dirty depends on which copy path the host CPU takes, which is why the failure comes and goes between CI runners.mysql.ibdis the only datadir file larger than one 4 MiB lz4 block (i.e. with enough literal-run/match sequences to hit the window), which is why it's always that file.Evidence, from a local reproduction that mirrors the Vitess pipeline against a real 28 MB
mysql.ibd: stock v2.6.1 on amd64 fails 4/20 runs with the exact CI error (independent of writer concurrency and compression level); the same seeds never fail on arm64 (pure-Go decoder); the compressed frames are byte-identical on both architectures and decode correctly with both the arm64 decoder and the v4 reader — so the backups are valid; only the amd64 decoder is broken, and existing lz4 backups remain restorable after this upgrade. Patching the one instruction (spilling the full register) also makes the failure vanish, 0/40.It turns out this exact bug was already root-caused upstream after we drafted this: golang/go#74925 hit the same error after upgrading to Go 1.24, and Keith Randall pinpointed the very same instruction — "MOVB does not clear the upper 56 bits. You would need MOVBQZX. v4 does this store/load with MOVL" — verifying that v2.6.1 (and v3.3.5) fail while v4 doesn't, and closing the issue as not-a-Go-bug. The fix on the v4 line is pierrec/lz4@3135ebdb71 ("Less stack spilling in memmove calls", merged via pierrec/lz4#120, first shipped in v4.1.4): the
MOVBspill/restore of the token register becameMOVL, which zero-extends on restore and clears the garbage. It was a performance refactor whose correctness fix was accidental, which is also why it was never backported to v2. The thread additionally explains why this started flaking in CI now: Go ≥ 1.24 (golang/go@601ea46a53) added FSRM/ERMS-vectorizedmemmovepaths on capable Intel CPUs that leave different garbage in RDX — AMD and older Intel runners are unaffected, hence the runner-to-runner variance. pierrec/lz4#233 tracks the v2/v3 exposure upstream.v2.6.1 is the final v2 release, so the fix is moving to the maintained
pierrec/lz4/v4(v4.1.27). The reader is a drop-in; the writer moves from theHeaderstruct to v4 options. One wrinkle: v4 takes a level enum rather than v2's raw hash-chain search depth, and rejects other values. The new mapping keeps--compression-level0 and 1 (including the default 1, whose v2 depth of 1 did almost no searching) on the fast compressor, maps 2–9 ontoLevel2–Level9, and maps negative values ontoLevel9— v2 treated a negative level as an unlimited hash-chain search, so the deepest named level is the faithful translation.A second wrinkle surfaced in this PR's own CI run (
Cluster (xb_backup)): v4's writer implementsReadFrom, but only for a fresh writer, andio.CopyNselects the destination'sReadFrombecauseLimitReaderhides the source'sWriteTo. The striped xtrabackup backup round-robinsio.CopyNover its destination writers, so the second block per stripe failed the whole backup withlz4: unhandled state[writeState](upstream knows this API is fragile: pierrec/lz4#183). The lz4 compressor and decompressor are now wrapped in types that hideReadFrom/WriteTo, keeping every copy on the plainWrite/Readpath, andTestBuiltinCompressorsSequentialCopyNpins the striped-copy shape for all builtin engines.A new
TestBuiltinCompressorsMultiBlockround-trips a 9 MiB mixed compressible/incompressible payload through every builtin engine — the multi-block, long-literal-run shape that exposed this, which had no coverage (the existing round-trip test uses 14 bytes). An honest in-tree regression test for the decoder bug itself isn't possible: the trigger depends on the host CPU'smemmovepath, so it would pass onmainon most machines. The dependency version is the guard.Backport justification
This should go to both supported release branches (release-23.0, release-24.0):
pierrec/lz4 v2.6.1, so every current release binary carries the live bug on FSRM-capable Intel CPUs.--compression-level2–9 mapping described in the deployment notes; the default profile is preserved.GODEBUG=cpu.fsrm=offavoids the Go ≥ 1.24 trigger path (the workaround from Issue with block decoding and vectorized runtime.memmove (Go >= 1.24) pierrec/lz4#233), though the bug stays latent.Related Issue(s)
None found — searches for the error string and the test name came up empty.
Checklist
Deployment Notes
For users running backups with
--compression-engine-name=lz4(not the default; the default engine ispargzip):lz4: invalid source or destination buffer too shortdepending on the host CPU. Fixed by this upgrade. Backups written by older Vitess versions are unaffected and remain restorable — the frame format is unchanged.--compression-levelvalues 2–9 now select lz4's named hash-chain levels (search depths 1024–131072) instead of using the raw value as the search depth, so higher levels get a better ratio at more CPU. Values 0 and 1, including the default, keep the current fast profile; negative values keep selecting the deepest search (Level9, depth 131072, versus v2's unlimited search capped at the 65536-byte window).AI Disclosure
The investigation (assembly-level root cause, local reproduction harness, and cross-architecture validation) and the fix were done by Claude Code; I provided direction.