Skip to content

fix: set ZSTD Frame_Content_Size per record#203

Merged
NGTmeaty merged 4 commits into
masterfrom
feat/zstd-frame-content-size
Jul 7, 2026
Merged

fix: set ZSTD Frame_Content_Size per record#203
NGTmeaty merged 4 commits into
masterfrom
feat/zstd-frame-content-size

Conversation

@yzqzss

@yzqzss yzqzss commented Jun 28, 2026

Copy link
Copy Markdown
Collaborator

close: #202


The unitest was written by AI.

@yzqzss
yzqzss requested review from NGTmeaty and willmhowes June 28, 2026 04:17
Comment thread write.go

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the Zstandard WARC writer to ensure each compressed WARC record is written as a ZSTD frame that includes the spec-required Frame_Content_Size, addressing issue #202 and improving spec compliance for larger records.

Changes:

  • Introduces a sizedZstdWriter wrapper to set ZSTD Frame_Content_Size per record.
  • Updates Writer.WriteRecord to serialize a full record (header + body + trailer) and pre-set the compressor content size before encoding.
  • Adds an end-to-end test that parses produced ZSTD frames and asserts Frame_Content_Size and Content_Checksum are present.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
zstd_writer.go Adds a ZSTD encoder wrapper that can set Frame_Content_Size for the next frame/record.
write.go Serializes records into a single reader stream and sets per-record ZSTD content size before writing.
write_zstd_e2e_test.go Adds e2e coverage asserting generated ZSTD frames include required header fields and are checksum-verified.
utils.go Switches getContentLength return type to int64 and updates ZSTD writer construction to use the new wrapper.
dialer.go Updates Content-Length header formatting to match int64 size handling.
dedupe.go Updates record size parsing/storage to int64.
Comments suppressed due to low confidence (1)

utils.go:208

  • getContentLength() currently seeks and reads the full in-memory buffer into a new bytes.Buffer to compute the size. That has two concrete downsides: (1) it can move the underlying read cursor (callers that call getContentLength twice can get 0 the second time for in-memory records), and (2) it forces an O(n) read+copy even though spooledtempfile already exposes Len() for an O(1) size check. Prefer rwsc.Len() and only fall back to the current seek+read/stat behavior if Len() fails.
func getContentLength(rwsc spooledtempfile.ReadWriteSeekCloser) int64 {
	// If the FileName leads to no existing file, it means that the SpooledTempFile
	// never had the chance to buffer to disk instead of memory, in which case we can
	// just read the buffer (which should be <= 2MB) and return the length
	if rwsc.FileName() == "" {

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

yzqzss added a commit to saveweb/gowarc that referenced this pull request Jun 28, 2026

@NGTmeaty NGTmeaty left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you again for catching and submitting the PR! A few comments but otherwise I think it's good to merge!

Comment thread write.go Outdated
Comment thread write.go
Comment thread write.go
Comment thread write.go
Comment thread write.go
Comment thread write.go Outdated
@yzqzss

yzqzss commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

Sorry I missed your comments, GitHub didn't send me a notification for some reasons :(
I'll take a look later.

@NGTmeaty

NGTmeaty commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

No worries, I meant to message you as well, oops!

@NGTmeaty NGTmeaty left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks great! I'll get this merged in tonight!

@NGTmeaty
NGTmeaty merged commit 62df20e into master Jul 7, 2026
6 checks passed
@NGTmeaty
NGTmeaty deleted the feat/zstd-frame-content-size branch July 7, 2026 05:57
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.

zstd warcs generated by gowarc do not have the Frame_Content_Size set

3 participants