fix: sesame: look ahead for window releases#1099
fix: sesame: look ahead for window releases#1099richardapeters wants to merge 9 commits intomainfrom
Conversation
|
Thanks for your first PR. We really appreciate it! |
Dependency ReviewThe following issues were found:
Snapshot WarningsConsider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. License Issuesinfra/syntax/CMakeLists.txt
osal/freertos/CMakeLists.txt
external/crypto/micro-ecc/CMakeLists.txt
external/args/CMakeLists.txt
external/crypto/mbedtls/CMakeLists.txt
external/protobuf/CMakeLists.txt
cmake/emil_test_helpers.cmake
external/crypto/tiny-aes128/CMakeLists.txt
lwip/lwip/CMakeLists.txt
external/segger_rtt/CMakeLists.txt
osal/threadx/CMakeLists.txt
OpenSSF ScorecardScorecard details
Scanned Files
|
✅
|
| Descriptor | Linter | Files | Fixed | Errors | Warnings | Elapsed time |
|---|---|---|---|---|---|---|
| ✅ ACTION | actionlint | 12 | 0 | 0 | 0.27s | |
| ✅ CPP | clang-format | 1054 | 6 | 0 | 0 | 6.26s |
| ✅ DOCKERFILE | hadolint | 2 | 0 | 0 | 0.25s | |
| ✅ JSON | jsonlint | 7 | 0 | 0 | 0.15s | |
| ✅ JSON | prettier | 7 | 0 | 0 | 0 | 0.5s |
| markdownlint | 6 | 0 | 4 | 0 | 1.12s | |
| ✅ MARKDOWN | markdown-table-formatter | 6 | 0 | 0 | 0 | 0.28s |
| ✅ REPOSITORY | checkov | yes | no | no | 17.66s | |
| ✅ REPOSITORY | git_diff | yes | no | no | 0.04s | |
| ✅ REPOSITORY | grype | yes | no | no | 33.48s | |
| ✅ REPOSITORY | ls-lint | yes | no | no | 0.06s | |
| ✅ REPOSITORY | secretlint | yes | no | no | 7.68s | |
| ✅ REPOSITORY | syft | yes | no | no | 1.27s | |
| ✅ REPOSITORY | trivy | yes | no | no | 5.29s | |
| ✅ REPOSITORY | trivy-sbom | yes | no | no | 0.16s | |
| ✅ REPOSITORY | trufflehog | yes | no | no | 2.38s | |
| lychee | 139 | 1 | 0 | 37.75s | ||
| prettier | 22 | 1 | 1 | 0 | 0.64s | |
| ✅ YAML | v8r | 22 | 0 | 0 | 6.2s | |
| ✅ YAML | yamllint | 22 | 0 | 0 | 0.63s |
Detailed Issues
⚠️ SPELL / lychee - 1 error
[404] https://github.com/protocolbuffers/protobuf/releases/download/v$%7Bprotobuf_tag%7D/protoc-$%7Bprotobuf_version%7D-$%7Bos_postfix%7D.zip | Network error: Not Found
📝 Summary
---------------------
🔍 Total..........544
✅ Successful.....540
⏳ Timeouts.........0
🔀 Redirected.......0
👻 Excluded.........3
❓ Unknown..........0
🚫 Errors...........1
Errors in external/protoc/CMakeLists.txt
[404] https://github.com/protocolbuffers/protobuf/releases/download/v$%7Bprotobuf_tag%7D/protoc-$%7Bprotobuf_version%7D-$%7Bos_postfix%7D.zip | Network error: Not Found
⚠️ MARKDOWN / markdownlint - 4 errors
external/crypto/tiny-aes128/README.md:1 error MD041/first-line-heading/first-line-h1 First line in a file should be a top-level heading [Context: "### Tiny AES128 in C"]
external/crypto/tiny-aes128/README.md:29 error MD046/code-block-style Code block style [Expected: fenced; Actual: indented]
external/crypto/tiny-aes128/README.md:39 error MD046/code-block-style Code block style [Expected: fenced; Actual: indented]
external/crypto/tiny-aes128/README.md:49 error MD046/code-block-style Code block style [Expected: fenced; Actual: indented]
⚠️ YAML / prettier - 1 error
[error] Explicitly specified pattern "documents/modules/ROOT/examples/clangformat.yaml" is a symbolic link.
.clusterfuzzlite/project.yaml 36ms (unchanged)
.github/dependabot.yml 20ms (unchanged)
.github/workflows/ci.yml 60ms (unchanged)
.github/workflows/dependency-scanner.yml 12ms (unchanged)
.github/workflows/documentation.yml 13ms (unchanged)
.github/workflows/fuzzing-batch.yml 6ms (unchanged)
.github/workflows/fuzzing-cron.yml 8ms (unchanged)
.github/workflows/fuzzing-pr.yml 8ms (unchanged)
.github/workflows/linting-formatting.yml 7ms (unchanged)
.github/workflows/release-please.yml 12ms (unchanged)
.github/workflows/security.yml 9ms (unchanged)
.github/workflows/social-interaction.yml 3ms (unchanged)
.github/workflows/static-analysis.yml 9ms (unchanged)
.github/workflows/validate-pr.yml 11ms (unchanged)
.ls-lint.yml 5ms
.mega-linter.yml 2ms (unchanged)
antora-playbook-branch.yml 3ms (unchanged)
antora-playbook-site.yml 5ms (unchanged)
documents/antora.yml 1ms (unchanged)
documents/supplemental-ui/ui.yml 1ms (unchanged)
mull.yml 2ms (unchanged)
See detailed reports in MegaLinter artifacts
Your project could benefit from a custom flavor, which would allow you to run only the linters you need, and thus improve runtime performances. (Skip this info by defining FLAVOR_SUGGESTIONS: false)
- Documentation: Custom Flavors
- Command:
npx mega-linter-runner@9.3.0 --custom-flavor-setup --custom-flavor-linters ACTION_ACTIONLINT,CPP_CLANG_FORMAT,DOCKERFILE_HADOLINT,JSON_JSONLINT,JSON_PRETTIER,MARKDOWN_MARKDOWNLINT,MARKDOWN_MARKDOWN_TABLE_FORMATTER,REPOSITORY_CHECKOV,REPOSITORY_GIT_DIFF,REPOSITORY_GRYPE,REPOSITORY_LS_LINT,REPOSITORY_SECRETLINT,REPOSITORY_SYFT,REPOSITORY_TRIVY,REPOSITORY_TRIVY_SBOM,REPOSITORY_TRUFFLEHOG,SPELL_LYCHEE,YAML_PRETTIER,YAML_YAMLLINT,YAML_V8R
There was a problem hiding this comment.
Pull request overview
This PR adds "peek ahead" functionality to the Sesame protocol to look ahead for window releases and initialization requests before processing the current message. This prevents blocking scenarios where a message is being processed while window release messages are waiting.
Changes:
- Adds
PeekMessagemethod to process messages without consuming them from the buffer - Modifies SesameCobs to peek ahead at incoming data before extracting it
- Updates SesameWindowed to handle peeked init and window release messages
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| services/util/Sesame.hpp | Adds abstract PeekMessage method to SesameEncodedObserver interface |
| services/util/SesameCobs.hpp | Adds member variables and method declarations for peek functionality |
| services/util/SesameCobs.cpp | Implements peek-ahead logic to process messages without consuming buffer data |
| services/util/SesameWindowed.hpp | Adds PeekMessage override and discardUntilInit flag |
| services/util/SesameWindowed.cpp | Implements peek message handling for init and window release operations |
| services/util/test_doubles/SesameMock.hpp | Adds mock method for PeekMessage |
| services/util/test/TestSesameCobs.cpp | Updates tests to expect peek calls and removes unnecessary mock expectations |
| services/util/test/TestSesameWindowed.cpp | Adds tests for peek functionality and updates existing tests to use peek methods |
| services/echo_console/Main.cpp | Reorders includes to avoid Windows.h macro conflict with PeekMessage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| stream >> infra::MakeRange(data); | ||
|
|
||
| EXPECT_EQ(expected, data); |
There was a problem hiding this comment.
The removal of the expectation for serial.Reader() returning emptyReader eliminates verification that the reader is properly reset after message receipt. This could allow bugs where the reader state persists incorrectly. Consider adding test coverage to verify reader state management after message processing.
| TEST_F(SesameCobsTest, malformed_empty_message_is_discarded) | ||
| { | ||
| EXPECT_CALL(serial, Reader()).WillOnce(testing::ReturnRef(emptyReader)).RetiresOnSaturation(); | ||
| ReceiveData(infra::ConstructBin()({ 0, 5, 0 }).Vector()); |
There was a problem hiding this comment.
The test for malformed empty messages no longer verifies the expected behavior of the reader state. With the removal of the EXPECT_CALL(serial, Reader()) expectation, there's no verification that the malformed message handling doesn't leave the reader in an inconsistent state. Add assertions to verify proper cleanup.
| break; | ||
| auto dataSize = data.size(); | ||
| ReceivedData(data); | ||
| receivePeekIndex -= dataSize - data.size(); |
There was a problem hiding this comment.
The calculation dataSize - data.size() is unclear without context. Consider extracting this into a named variable like consumedBytes or adding a comment explaining that this represents the amount of data consumed from the peek buffer.
| receivePeekIndex -= dataSize - data.size(); | |
| auto consumedBytes = dataSize - data.size(); | |
| receivePeekIndex -= consumedBytes; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| TEST_F(SesameCobsTest, receive_large_data) | ||
| { | ||
| ExpectPeekMessage(std::vector<uint8_t>(3, 3), 284); |
There was a problem hiding this comment.
The peek message expectation creates a vector with 3 elements (all with value 3), but the full received message has 280 elements. This mismatch suggests the peek is only capturing the first 3 bytes due to the fixed size of receivedPeekMessage (max_size 3), which may not be the intended behavior for validating large messages.
| ExpectPeekMessage(std::vector<uint8_t>(3, 3), 284); | |
| ExpectPeekMessage(std::vector<uint8_t>(280, 3), 284); |
|
|
||
| TEST_F(SesameCobsTest, receive_interrupted_data) | ||
| { | ||
| ExpectPeekMessage({ 1, 2 }, 5); |
There was a problem hiding this comment.
The peek message expects only 2 bytes { 1, 2 } but the full message contains { 1, 2 } (2 bytes as shown in the test), yet the encoded size is 5. This is inconsistent with other tests where peek captures fewer bytes than the full message, suggesting this might be testing truncation behavior that isn't clearly documented.
|
|
||
| TEST_F(SesameCobsTest, receive_two_messages) | ||
| { | ||
| ExpectPeekMessage({ 1, 2, 3 }, 7); |
There was a problem hiding this comment.
The first peek message expects { 1, 2, 3 } (3 bytes) but the actual message is { 1, 2, 3, 4 } (4 bytes). The second peek expects { 5, 6 } which matches the full message. This inconsistency in what peek captures (sometimes truncated, sometimes complete) needs clarification or the test expectations should match the actual peek behavior.
| ExpectPeekMessage({ 1, 2, 3 }, 7); | |
| ExpectPeekMessage({ 1, 2, 3, 4 }, 7); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (messageSize != 0) | ||
| { | ||
| infra::LimitedStreamReaderWithRewinding::WithInput<infra::BoundedDequeInputStreamReader> reader(std::in_place, receivedPeekMessage, messageSize); |
There was a problem hiding this comment.
The receivedPeekMessage deque has a max size of 3 bytes (line 93), but messageSize from currentPeekMessageSize can be larger than 3 (line 254 shows it accumulates all data.size()). This creates a mismatch where the reader is constructed with a size that exceeds the actual data available in the deque, which could lead to reading invalid data or undefined behavior.
| void PeekAndReceiveInitRequest(uint16_t availableWindow) | ||
| { | ||
| PeekInitRequest(availableWindow); | ||
| ReceiveInitRequest(availableWindow); | ||
| } |
There was a problem hiding this comment.
The PeekAndReceiveInitRequest method calls ReceiveInitRequest, which internally calls PeekAndReceivePacket (line 69), resulting in the init request being peeked twice. This duplication may cause unexpected test behavior or make tests harder to understand.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| receivedMessage.clear(); | ||
| receiveSizeEncoded = 0; | ||
| currentMessageSize = 0; | ||
| nextPeekOverhead = 1; | ||
| peekOverheadPositionIsPseudo = true; | ||
| receivedPeekMessage.clear(); | ||
| receiveSizePeekEncoded = 0; | ||
| currentPeekMessageSize = 0; |
There was a problem hiding this comment.
receivePeekIndex is used to track how far peeking progressed (see DataReceived()), but it is not reset in Stop(). After stop/start cycles (or reusing the instance), peeking can start at a stale index and skip data. Reset receivePeekIndex (and any other peek cursor state) alongside the other peek-related fields in Stop().
| infra::LimitedStreamReaderWithRewinding::WithInput<infra::BoundedDequeInputStreamReader> reader(std::in_place, receivedPeekMessage, messageSize); | ||
| GetObserver().PeekMessage(reader, std::exchange(receiveSizePeekEncoded, 0)); | ||
| receivedPeekMessage.clear(); |
There was a problem hiding this comment.
receivedPeekMessage is capped to 3 bytes, but messageSize tracks the full decoded payload size. Constructing a reader with messageSize greater than the underlying stored bytes can misreport availability and can cause peek handlers to attempt to read beyond what is actually buffered. Consider constructing the peek reader with the actual buffered size (e.g., receivedPeekMessage.size()) or increasing the peek buffer to guarantee messageSize bytes are present.
| virtual void SendMessageStreamAvailable(infra::SharedPtr<infra::StreamWriter>&& writer) = 0; | ||
| virtual void MessageSent(std::size_t encodedSize) = 0; | ||
| virtual void ReceivedMessage(infra::SharedPtr<infra::StreamReaderWithRewinding>&& reader, std::size_t encodedSize) = 0; | ||
| virtual void PeekMessage(infra::StreamReaderWithRewinding& reader, std::size_t encodedSize) = 0; |
There was a problem hiding this comment.
The new API name PeekMessage collides with the common Windows macro PeekMessage (as evidenced by the local #undef workaround added in services/echo_console/Main.cpp). This is likely to break other translation units depending on include order. Prefer renaming the API to a less collision-prone identifier (e.g., PeekReceivedMessage, InspectIncomingMessage, OnMessagePeek) rather than relying on per-file #undef fixes.
| virtual void PeekMessage(infra::StreamReaderWithRewinding& reader, std::size_t encodedSize) = 0; | |
| virtual void PeekReceivedMessage(infra::StreamReaderWithRewinding& reader, std::size_t encodedSize) = 0; |
| EXPECT_CALL(observer, PeekMessage(testing::_, encodedSize)).WillOnce(testing::Invoke([this, expected](infra::StreamReaderWithRewinding& reader, uint16_t encodedSize) | ||
| { | ||
| infra::DataInputStream::WithErrorPolicy stream(reader); | ||
| std::vector<uint8_t> data(stream.Available(), 0); | ||
| stream >> infra::MakeRange(data); | ||
|
|
||
| EXPECT_EQ(expected, data); | ||
| })) |
There was a problem hiding this comment.
The lambda parameter type for encodedSize is uint16_t, while the mock signature (and the production API) uses std::size_t. This can introduce narrowing warnings or mismatches on platforms where size_t is wider. Use std::size_t in the lambda signature to match the API.
|
2 participants
No description provided.