Skip to content

Fix checksum validation for multipart ZIM files - #1135

Merged
veloman-yunkan merged 1 commit into
openzim:mainfrom
VikramAditya33:fix-multipart-checksum-validation
Sep 15, 2026
Merged

veloman-yunkan merged 1 commit into
openzim:mainfrom
VikramAditya33:fix-multipart-checksum-validation

Conversation

@VikramAditya33

@VikramAditya33 VikramAditya33 commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

This change calculates the checksum through zimReader over the logical range from the beginning of the archive to the stored checksum. Multipart traversal, physical offsets, and part sizes are therefore handled by the existing reader abstraction.

The regression test creates a valid ZIM and embeds it as several non-aligned parts with unrelated bytes before and between them. The test fails with the previous implementation and passes with this fix.

Using the 64KiB buffer, on a 1 GiB ZIM stored in tmpfs, seven validation runs improved from a median of 2.145 seconds at approximately 477 MiB/s to 1.554 seconds at approximately 659 MiB/s which is a 27.6% reduction in validation time, or about a 1.38x speedup.

Fixes #1131

@VikramAditya33

Copy link
Copy Markdown
Contributor Author

@kelson42 @veloman-yunkan PTAL

@codecov

codecov Bot commented Sep 14, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 63.63636% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.76%. Comparing base (6a49d7d) to head (afdde72).

Files with missing lines Patch % Lines
src/fileimpl.cpp 63.63% 1 Missing and 3 partials ⚠️

❌ Your patch check has failed because the patch coverage (63.63%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1135      +/-   ##
==========================================
+ Coverage   56.63%   56.76%   +0.12%     
==========================================
  Files         100      100              
  Lines        5064     5056       -8     
  Branches     2246     2239       -7     
==========================================
+ Hits         2868     2870       +2     
+ Misses        715      714       -1     
+ Partials     1481     1472       -9     

☔ View full report in Codecov by Harness.
📢 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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

All reviewed files are covered, with no unresolved blocking issues.

Pull request overview

Fixes checksum validation for multipart and embedded ZIM files by hashing through the logical reader view.

Changes:

  • Uses a 64 KiB buffer for checksum validation.
  • Centralizes the checksum buffer-size constant.
  • Adds regression coverage for non-aligned multipart archives.
File summaries
File Description
test/archive.cpp Adds multipart checksum regression coverage.
src/writer/creator.cpp Uses the shared buffer constant.
src/fileimpl.cpp Hashes the logical archive range through zimReader.
src/constants.h Defines the shared checksum buffer size.
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread test/archive.cpp Resolved
Comment thread test/archive.cpp Resolved
@VikramAditya33
VikramAditya33 force-pushed the fix-multipart-checksum-validation branch from cfc7dcc to afdde72 Compare September 14, 2026 14:48
@VikramAditya33

Copy link
Copy Markdown
Contributor Author

@veloman-yunkan I've simplified the tests as suggested.

Comment thread test/archive.cpp Resolved
@veloman-yunkan

Copy link
Copy Markdown
Collaborator

@VikramAditya33 Please update the description of the PR to agree with the actual code.

@veloman-yunkan
veloman-yunkan merged commit ae0212c into openzim:main Sep 15, 2026
49 of 53 checks passed
@veloman-yunkan

Copy link
Copy Markdown
Collaborator

Thank you for your contribution!

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.

Checksum validation is buggy for fd-based ZIM archives

3 participants