feat: S08.10 tcp_reconnect BDD scenario green on FreeRTOS QEMU#342
Conversation
Split @Buffered into @Buffered (interactive process) + @store (file- backed store) so the FreeRTOS compose filter admits tcp_reconnect without dragging in store_and_forward / capacity_threshold / store_capacity / block_lifecycle / power_cycle_replay. Route the buffered-interactive spawn through target_driver so QEMU plus `set transport tcp` over the UART falls out for free on FreeRTOS; Linux/Windows argv path unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors buffered BDD target process startup by extracting configuration flag building into a ChangesBuffered BDD Target Startup Refactoring with Store-Tag Integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1111 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Bdd/features/steps/syslog_steps.py`:
- Around line 507-514: The current truthy getattr checks when building extra
args drop explicit zero values (e.g., --capacity-threshold 0); update the checks
for numeric fields store_max_blocks, store_max_block_size, and
capacity_threshold to use "is not None" (e.g., getattr(context,
'store_max_blocks', None) is not None) so zero is preserved when appending to
args, while keeping the existing truthy check for the string field
store_discard_policy as-is.
In `@DEVLOG.md`:
- Around line 129-155: Replace the literal asterisk-number tokens that are being
parsed as emphasis (e.g. "*40", "*48", the sequence "*16 → *32 → *40 → *48", and
any occurrences in "SOLIDSYSLOG_*_SIZE") with their code-literal form by
wrapping them in backticks (or escaping the asterisks) so the markdown renders
them as literal tokens; update the DEVLOG.md instances that mention the stack
budgets and the "project_freertos_stack_budget" rename accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45cea966-fe27-45dd-9290-554c25a5afaf
📒 Files selected for processing (8)
Bdd/features/block_lifecycle.featureBdd/features/capacity_threshold.featureBdd/features/power_cycle_replay.featureBdd/features/steps/syslog_steps.pyBdd/features/store_and_forward.featureBdd/features/store_capacity.featureDEVLOG.mdci/docker-compose.bdd.yml
| - **InteractiveTask stack *40 → *48.** StreamSender.Connect allocates | ||
| a SolidSyslogAddressStorage plus a SolidSyslogFormatter sized for | ||
| SOLIDSYSLOG_MAX_HOST_SIZE on stack; TransmitFramed adds a small | ||
| octet-prefix formatter. Empirically tipped a *40 budget into a | ||
| Cortex-M lockup (PC=0, fault-during-fault — classic | ||
| stack-overflow-corrupting-the-exception-return signature) when | ||
| SwitchingSender flipped UDP→TCP→UDP across repeated sends. *48 | ||
| restored ~4 KB headroom and the same scenario completed cleanly. | ||
| - **Test fixture: openStream() / readIntoBuffer() helpers and a | ||
| CHECK_SOCKET_CLOSED_ONCE macro.** The arrange step in 25+ tests | ||
| collapses to `openStream();`. The macro pins both the closesocket | ||
| call count *and* the identity of the closed socket — applying it | ||
| uniformly across the close-on-failure tests caught three sites | ||
| (SendClosesSocketOnError, ReadReturnsNegativeOneOnErrorAndClosesSocket, | ||
| DestroyClosesOpenSocket) that previously only checked the count. | ||
| The check count went 56 → 59 from that tightening alone. | ||
|
|
||
| ### Deferred | ||
|
|
||
| - **CMake-driven memory scaling** for both static-`_Create` storage | ||
| sizes and FreeRTOS task stack depths. The recurring stack bumps | ||
| (*16 → *32 → *40 → *48) and the per-class `SOLIDSYSLOG_*_SIZE` | ||
| macros point to this as the right next intervention: expose them as | ||
| CMake-tunable variables so integrators size memory once, | ||
| declaratively, instead of editing `main.c` each time a new feature | ||
| lands. Captured in memory `project_freertos_stack_budget` (renamed | ||
| from *40 to *48 ceiling). |
There was a problem hiding this comment.
Fix malformed emphasis markers in stack-size text.
The *N tokens are being parsed as emphasis and trigger markdownlint MD037 warnings (Line 135, Line 150, Line 155). Wrap these values in backticks (or escape *) to keep the intended literal text.
Suggested patch
-- **InteractiveTask stack *40 → *48.** StreamSender.Connect allocates
+- **InteractiveTask stack `*40` → `*48`.** StreamSender.Connect allocates
- (*16 → *32 → *40 → *48) and the per-class `SOLIDSYSLOG_*_SIZE`
+ (`*16` → `*32` → `*40` → `*48`) and the per-class `SOLIDSYSLOG_*_SIZE`
- from *40 to *48 ceiling).
+ from `*40` to `*48` ceiling).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **InteractiveTask stack *40 → *48.** StreamSender.Connect allocates | |
| a SolidSyslogAddressStorage plus a SolidSyslogFormatter sized for | |
| SOLIDSYSLOG_MAX_HOST_SIZE on stack; TransmitFramed adds a small | |
| octet-prefix formatter. Empirically tipped a *40 budget into a | |
| Cortex-M lockup (PC=0, fault-during-fault — classic | |
| stack-overflow-corrupting-the-exception-return signature) when | |
| SwitchingSender flipped UDP→TCP→UDP across repeated sends. *48 | |
| restored ~4 KB headroom and the same scenario completed cleanly. | |
| - **Test fixture: openStream() / readIntoBuffer() helpers and a | |
| CHECK_SOCKET_CLOSED_ONCE macro.** The arrange step in 25+ tests | |
| collapses to `openStream();`. The macro pins both the closesocket | |
| call count *and* the identity of the closed socket — applying it | |
| uniformly across the close-on-failure tests caught three sites | |
| (SendClosesSocketOnError, ReadReturnsNegativeOneOnErrorAndClosesSocket, | |
| DestroyClosesOpenSocket) that previously only checked the count. | |
| The check count went 56 → 59 from that tightening alone. | |
| ### Deferred | |
| - **CMake-driven memory scaling** for both static-`_Create` storage | |
| sizes and FreeRTOS task stack depths. The recurring stack bumps | |
| (*16 → *32 → *40 → *48) and the per-class `SOLIDSYSLOG_*_SIZE` | |
| macros point to this as the right next intervention: expose them as | |
| CMake-tunable variables so integrators size memory once, | |
| declaratively, instead of editing `main.c` each time a new feature | |
| lands. Captured in memory `project_freertos_stack_budget` (renamed | |
| from *40 to *48 ceiling). | |
| - **InteractiveTask stack `*40` → `*48`.** StreamSender.Connect allocates | |
| a SolidSyslogAddressStorage plus a SolidSyslogFormatter sized for | |
| SOLIDSYSLOG_MAX_HOST_SIZE on stack; TransmitFramed adds a small | |
| octet-prefix formatter. Empirically tipped a *40 budget into a | |
| Cortex-M lockup (PC=0, fault-during-fault — classic | |
| stack-overflow-corrupting-the-exception-return signature) when | |
| SwitchingSender flipped UDP→TCP→UDP across repeated sends. *48 | |
| restored ~4 KB headroom and the same scenario completed cleanly. | |
| - **Test fixture: openStream() / readIntoBuffer() helpers and a | |
| CHECK_SOCKET_CLOSED_ONCE macro.** The arrange step in 25+ tests | |
| collapses to `openStream();`. The macro pins both the closesocket | |
| call count *and* the identity of the closed socket — applying it | |
| uniformly across the close-on-failure tests caught three sites | |
| (SendClosesSocketOnError, ReadReturnsNegativeOneOnErrorAndClosesSocket, | |
| DestroyClosesOpenSocket) that previously only checked the count. | |
| The check count went 56 → 59 from that tightening alone. | |
| ### Deferred | |
| - **CMake-driven memory scaling** for both static-`_Create` storage | |
| sizes and FreeRTOS task stack depths. The recurring stack bumps | |
| (`*16` → `*32` → `*40` → `*48`) and the per-class `SOLIDSYSLOG_*_SIZE` | |
| macros point to this as the right next intervention: expose them as | |
| CMake-tunable variables so integrators size memory once, | |
| declaratively, instead of editing `main.c` each time a new feature | |
| lands. Captured in memory `project_freertos_stack_budget` (renamed | |
| from `*40` to `*48` ceiling). |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 135-135: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
[warning] 150-150: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
[warning] 150-150: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
[warning] 155-155: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@DEVLOG.md` around lines 129 - 155, Replace the literal asterisk-number tokens
that are being parsed as emphasis (e.g. "*40", "*48", the sequence "*16 → *32 →
*40 → *48", and any occurrences in "SOLIDSYSLOG_*_SIZE") with their code-literal
form by wrapping them in backticks (or escaping the asterisks) so the markdown
renders them as literal tokens; update the DEVLOG.md instances that mention the
stack budgets and the "project_freertos_stack_budget" rename accordingly.
There was a problem hiding this comment.
Declining — those *40/*48/*16 tokens are in the S08.09 DEVLOG entry (commit 712bde8, PR #341, already merged) which this PR does not touch. Per the project SKILL.md convention "Never rewrite history. Always append.", historical DEVLOG entries are not edited even to fix lint warnings. The S08.10 entry added in this PR doesn't use bare *N tokens, so no new lint warnings are introduced. Future entries will avoid the pattern (use backticks around \*N`` or call them out as e.g. "INTERACTIVE_TASK_STACK_DEPTH = configMINIMAL_STACK_SIZE * 48").
There was a problem hiding this comment.
@DavidCozens Acknowledged — makes sense to keep history immutable per SKILL.md. I’ve double-checked: the S08.10 DEVLOG chunk in this PR doesn’t introduce any bare “*N” tokens, so no new MD037 warnings are added.
If you’d like to silence the legacy warnings without touching prior entries, we can:
- Add DEVLOG.md to markdownlint-cli2 ignores (repo-wide), or
- Add a rule override for MD037 scoped to DEVLOG.md.
Happy to open a tiny follow-up PR either way; just say which option you prefer (or leave as-is).
✏️ Learnings added
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 342
File: DEVLOG.md:129-155
Timestamp: 2026-05-11T19:55:23.240Z
Learning: Do not edit historical DEVLOG entries; follow SKILL.md “Never rewrite history. Always append.” For future DEVLOG entries, avoid bare *N tokens; use backticks (e.g., `*48`) or explicit expressions instead.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
`--capacity-threshold 0` is the documented "disable" sentinel for SolidSyslogStoreThresholdFunction; truthy-coerce would silently drop it. No scenario passes 0 today but the bug is latent in the helper. CodeRabbit catch on #342. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1111 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
1 participant
Summary
Closes #339. Second slice of S08.06 (TCP transport on FreeRTOS-Plus-TCP) — a verification slice on top of the S08.09 adapter + SwitchingSender:
@bufferedinto@buffered+@store. Earlier the tag meant both "uses the long-lived interactive process" and "needs a file-backed store"; the FreeRTOS compose filternot @bufferedthen excludedtcp_reconnect.featureeven though it doesn't need a file store.@bufferedkeeps the interactive-process meaning;@storemarks the file-store dependency.tcp_reconnect.featureis@tcp @buffered(no@store) so the FreeRTOS filter admits it.start_bdd_target_processthroughtarget_driver. The one-shot path already abstracted native-vs-QEMU and argv-vs-UART; the interactive path bypassed it with a parallelsubprocess.Popen. Unifying both means QEMU +set transport tcpover the UART falls out for free on FreeRTOS, and future embedded platforms extend one branch point instead of two.SendClosesSocketOnFailurealready covered by S08.09'sSendClosesSocketOnError(plusSendClosesSocketOnShortWriteandReadReturnsNegativeOneOnErrorAndClosesSocket, all usingCHECK_SOCKET_CLOSED_ONCE). The close-on-failure contract that makes reconnect possible is already pinned by three tests — no additional unit test added.Test plan
cmake --preset debug && ctest— 8/8 host suites green (incl.SolidSyslogFreeRtosTcpStreamTest39 tests).cmake --preset freertos-cross && cmake --build --preset freertos-cross --target SolidSyslogBddTarget— clean cross build.set transport tcpfollowed by repeatedsendthenquitexits cleanly via the UART — interactive task survives the SwitchingSender flip + Service-task retry storm against no listener.bdd-freertos-qemuCI job — the docker-side syslog-ng config swap is the real arbiter (can't be run locally; outside compose it breaks docker networking).bdd-linux-syslog-ng(tag rename touches@storefeatures; Linux filter still admits them).🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Documentation