Skip to content

Resulting report from BuildReport() was exceeding the provided maxSize limit#365

Merged
mengelbart merged 1 commit intopion:masterfrom
astroza:max-report-blocks-per-stream-fix
Sep 10, 2025
Merged

Resulting report from BuildReport() was exceeding the provided maxSize limit#365
mengelbart merged 1 commit intopion:masterfrom
astroza:max-report-blocks-per-stream-fix

Conversation

@astroza
Copy link
Contributor

@astroza astroza commented Sep 9, 2025

Description

Under packet loss stress, the rfc8888 interceptor was sending packets bigger than the maxSize limit, sometimes even bigger than the network MTU.

I found the next problems:

  1. Ranging over a map is not deterministic (this applies only for the testing code)

    for i, log := range r.streams {
        if len(r.streams) > 1 && int(i) == len(r.streams)-1 {
            maxReportBlocksPerStream = maxReportBlocks % len(r.streams)
        }
        block := log.metricsAfter(now, int64(maxReportBlocksPerStream))
        report.ReportBlocks = append(report.ReportBlocks, block)
    }
    

    maxReportBlocksPerStream could be updated in the last iteration as well as the first one

  2. r.streams is a "ssrc -> log" map, so the i variable will never be equal to len(r.streams)-1. i is not a array index

  3. maxReportBlocksPerStream as maxReportBlocks / (len(r.streams)-1) will exceed the blocks limit. This is specially true when len(r.streams) is a small number

  4. The tests were not catching the actual issue
    If you run the new test "MaxreportsPerStream 3 streams" with the old code, you will get

            	Error Trace:	/home/fastroza/src/interceptor/pkg/rfc8888/recorder_test.go:166
       	Error:      	"1746" is not less than "1200"
    

@codecov
Copy link

codecov bot commented Sep 9, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.63%. Comparing base (ff2807d) to head (86db425).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #365      +/-   ##
==========================================
- Coverage   78.71%   78.63%   -0.09%     
==========================================
  Files          82       82              
  Lines        5169     5163       -6     
==========================================
- Hits         4069     4060       -9     
- Misses        927      929       +2     
- Partials      173      174       +1     
Flag Coverage Δ
go 78.63% <100.00%> (-0.09%) ⬇️
wasm 76.07% <100.00%> (-0.03%) ⬇️

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.

@Sean-Der
Copy link
Member

Sean-Der commented Sep 9, 2025

Nice catch :)

@mengelbart what you think? If I don't hear back in a day or two @astroza I will merge myself!

@astroza astroza force-pushed the max-report-blocks-per-stream-fix branch 2 times, most recently from f0a46e9 to 45c8399 Compare September 9, 2025 22:48
@astroza astroza force-pushed the max-report-blocks-per-stream-fix branch from 45c8399 to 86db425 Compare September 9, 2025 23:36
Copy link
Contributor

@mengelbart mengelbart left a comment

Choose a reason for hiding this comment

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

Thanks @astroza!

@mengelbart mengelbart merged commit c770c72 into pion:master Sep 10, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants