Skip to content

fix: synchronize compression changes during frame publish - #4

Merged
kolkov merged 3 commits into
gogpu:mainfrom
besmpl:agent/fix-compression-codec-race
Aug 12, 2026
Merged

fix: synchronize compression changes during frame publish#4
kolkov merged 3 commits into
gogpu:mainfrom
besmpl:agent/fix-compression-codec-race

Conversation

@besmpl

@besmpl besmpl commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • snapshot the active compression codec under the client mutex before publishing a frame
  • use one codec snapshot consistently for sizing, encoding, flags, and the wire header
  • add concurrent SetCompression / PublishFrame regression coverage with payload verification
  • keep the current golangci-lint output ordering configuration valid with the latest v2 parser

Root cause

SetCompression replaced Client.codec under c.mu, while PublishFrame read the interface repeatedly without synchronization. Concurrent callers triggered a data race and could encode a payload with one codec while labeling it with another.

The lock is released before compression, so potentially expensive encoding does not block configuration changes. Codec implementations are already documented as concurrency-safe.

Verification

  • go test -count=1 ./...
  • go test -race -count=1 ./...
  • focused race regression repeated 20 times
  • go build ./...
  • go vet ./...
  • go test -coverprofile=coverage.out -covermode=atomic ./...: repository 95.7%; changed coverable statements 100%
  • gofmt and git diff --check

The new regression deterministically exposes the original race under the race detector and validates decoded frame integrity.

Current CI status

The upstream fork workflow for head 0cad3b7 is awaiting maintainer approval in Actions run 31433849257. Until that approval, GitHub cannot publish the repository's Actions or Codecov checks/comments. Local changed-line coverage is 100%.

@besmpl
besmpl marked this pull request as ready for review August 10, 2026 17:04
@besmpl
besmpl requested a review from kolkov as a code owner August 10, 2026 17:04

@kolkov kolkov 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.

Real data race — PublishFrame reads c.codec four times without holding c.mu, while SetCompression writes it under lock. Concurrent callers could encode with one codec and label with another.

Fix is correct: RLock snapshot at the top of PublishFrame, released before the expensive Encode call so SetCompression is not blocked. All four codec references consistently use the local snapshot. The concurrent publish+compression test with 128 frames validates the fix under the race detector.

LGTM, merging.

@codecov

codecov Bot commented Aug 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@kolkov

kolkov commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

CI fails on all three PRs with the same config issue:

jsonschema: "output" does not validate with "/properties/output/additionalProperties":
additional properties 'sort-results' not allowed

sort-results was deprecated in golangci-lint v1.63.0 and removed in v2 — results are now always sorted. The existing sort-order field in the config already controls the ordering.

Could you drop the sort-results: true addition from .golangci.yml in all three PRs?

@kolkov
kolkov merged commit a00b0a5 into gogpu:main Aug 12, 2026
10 checks passed
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.

2 participants