feat: S13.18 Windows BDD on the portable ring buffer#264
Conversation
Replace the select.select + os.read pattern in wait_for_prompt with a daemon thread that reads stdout byte-by-byte into a queue. Same Python signature; same return shape; works on POSIX and Windows pipe fds. Slice 1 of S13.18: prerequisite for letting the Windows runner drive buffered.feature and tcp_transport.feature via the same prompt protocol the Linux runner uses today. Verified: all six prompt-protocol-using features (buffered, tcp_transport, switching_transport, mtls_transport, tls_transport, block_lifecycle) pass against the Linux syslog-ng oracle.
Replace SolidSyslogNullBuffer with SolidSyslogCircularBuffer (10 messages, backed by SolidSyslogWindowsMutex) and add a Win32 service thread launched via _beginthreadex driving the existing ExampleServiceThread_Run. Shutdown-flag and WaitForSingleObject mirror the pthread join in the Linux Threaded example. Slice 2 of S13.18: prerequisite for letting the Windows BDD runner exercise the buffering path end-to-end. ExampleServiceThread.c now links into the Windows binary alongside the existing Common helpers. Verified: msvc-debug build clean, clang-format clean, IWYU clean (Windows-only file isn't in the Linux compile_commands.json so the gate is unaffected), Linux examples still build. Smoke-tested via a Python prompt-protocol harness — UDP datagram arrives on 127.0.0.1:5514 and the example exits cleanly.
Drop @Buffered from buffered.feature and tcp_transport.feature so they run on bdd-windows-otel — both now resolve to the buffered example prose, which routes to THREADED_BINARY on the Linux runner and the (S13.18) Win32-thread-driven binary on the Windows runner. Step layer: - Add run_buffered_example, the cross-platform sibling of run_threaded_example. Both share _run_with_prompt_protocol; the only difference is binary selection (oracle_format). - Add three new "the buffered example..." @when steps for the shapes used by the two cross-platform features. - Existing "the threaded example..." steps still resolve to THREADED_BINARY for the genuinely Linux-only features (file store, switching, mtls/tls, tcp_reconnect). tcp_singletask.feature: tag @windows_wip. Its purpose was to validate Windows TCP via the SingleTask/NullBuffer binary; post-S13.18 the Windows binary is buffered, and tcp_transport already covers Windows TCP. The single-task feature stays Linux-only as the bare-metal/synchronous-send pin. CI tag filter unchanged — `not @buffered` still excludes the right set on Windows after the @Buffered drop on the two cross-platform features. Verified: Linux BDD `not @wip` (16/16 features pass via syslog-ng); Windows-effective filter (`not @wip and not @windows_wip and not @buffered`) dry-run shows the new scenarios included and tcp_singletask excluded.
README, SKILL, and the bdd / iec62443 guides now reflect that SolidSyslogCircularBuffer is the cross-platform threaded buffer (replacing "planned" wording from S04.05) and that the Windows example is buffered + service-thread, not single-task. @Buffered tag description in docs/bdd.md rewritten to its post- S13.18 meaning: "needs a Linux-only buffered capability beyond a basic ring buffer + service thread" (file store, switching sender, TLS/mTLS, syslog-ng UNIX-socket reload). The cross- platform "single message via UDP/TCP through a real buffer" path is now untagged and runs on both runners. DEVLOG records the test-side-fix-over-production-semantics choice, the Windows example wiring, the @Buffered re-scoping, and the deferred refactors (CreateSender hoisting, drain-on- shutdown semantics, PosixMessageQueueBuffer retirement).
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (2)
📝 WalkthroughWalkthroughWindows BDD support is added for the portable ring buffer by refactoring cross-platform prompt handling in Python tests, updating the Windows example to use ChangesWindows Ring Buffer BDD & Example Integration
sequenceDiagram
participant Main as Main Thread
participant CB as CircularBuffer
participant MT as Mutex
participant ST as Service Thread
participant Send as Sender
Main->>CB: initialize with storage
Main->>MT: create WindowsMutex
Main->>ST: spawn via _beginthreadex
rect rgba(0, 100, 200, 0.5)
Main->>MT: lock
Main->>CB: enqueue message
Main->>MT: unlock
end
ST->>MT: lock
ST->>CB: dequeue message
ST->>MT: unlock
ST->>Send: send via syslog
Main->>Main: set shutdownFlag = true
Main->>ST: WaitForSingleObject (join)
Main->>MT: destroy
Main->>CB: destroy
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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. Review rate limit: 0/1 reviews remaining, refill in 42 minutes and 48 seconds.Comment |
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1049 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Bdd/features/steps/syslog_steps.py (1)
309-318:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRaise immediately if stdout closes before the prompt arrives.
When the queue yields
None, this helper returns partial output as if the handshake succeeded. That turns an example crash/early exit into a later timeout orBrokenPipeErrorin unrelated steps instead of failing at the real point of breakage.Suggested fix
try: data = process._solidsyslog_byte_queue.get(timeout=min(remaining, 0.5)) except queue.Empty: continue if data is None: - break + raise EOFError( + "Process exited before emitting the prompt. " + f"Output so far: {output.decode(errors='replace')}" + ) output += data if output.endswith(b"SolidSyslog> "): return output.decode() - return output.decode()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Bdd/features/steps/syslog_steps.py` around lines 309 - 318, When the helper reads from process._solidsyslog_byte_queue and sees data is None it should fail immediately instead of returning partial output; change the branch that currently breaks on None to raise an exception (e.g., RuntimeError with a clear message like "stdout closed before prompt") so callers see the real early-exit point; keep the rest of the loop (the queue.get, output accumulation, and the output.endswith(b"SolidSyslog> ") prompt check) unchanged.Example/Windows/SolidSyslogWindowsExample.c (1)
157-171:⚠️ Potential issue | 🟠 MajorCheck
_beginthreadex()return value before using as a HANDLE.
_beginthreadex()returns 0 on failure (not INVALID_HANDLE_VALUE). The current code does not check for failure, so if thread creation fails,serviceThreadbecomes a NULL handle. SubsequentWaitForSingleObject()andCloseHandle()calls will fail, and the service thread never runs—so buffered messages accumulate and never drain. Add an error check after line 158 and fail fast with cleanup instead of proceeding to the interactive loop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Example/Windows/SolidSyslogWindowsExample.c` around lines 157 - 171, Check the return value from _beginthreadex when assigning to serviceThread (it returns 0 on failure, not INVALID_HANDLE_VALUE); if _beginthreadex returns 0, set shutdownFlag=true, perform any necessary cleanup and fail fast instead of calling ExampleInteractive_Run. Specifically, after calling _beginthreadex that creates serviceThread for ServiceThreadEntry, verify serviceThread != 0, and on failure avoid calling ExampleInteractive_Run with an invalid handle and skip WaitForSingleObject/CloseHandle; log/report the error and cleanup resources before returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/iec62443.md`:
- Line 40: Update the SR 6.1 table row to stop claiming SolidSyslog_Log is
non-blocking for all buffers and instead state that non-blocking behavior
applies only when using buffered implementations (SolidSyslogCircularBuffer or
SolidSyslogPosixMessageQueueBuffer); explicitly note that SolidSyslogNullBuffer
performs sends on the caller's thread (synchronous single-task behavior) and
that SolidSyslog_Service performs blocking I/O on the integrator's chosen thread
bounded by SO_SNDTIMEO, referencing SolidSyslog_Log, SolidSyslogNullBuffer,
SolidSyslogCircularBuffer, SolidSyslogPosixMessageQueueBuffer, and
SolidSyslog_Service to clarify which components are synchronous vs buffered.
---
Outside diff comments:
In `@Bdd/features/steps/syslog_steps.py`:
- Around line 309-318: When the helper reads from
process._solidsyslog_byte_queue and sees data is None it should fail immediately
instead of returning partial output; change the branch that currently breaks on
None to raise an exception (e.g., RuntimeError with a clear message like "stdout
closed before prompt") so callers see the real early-exit point; keep the rest
of the loop (the queue.get, output accumulation, and the
output.endswith(b"SolidSyslog> ") prompt check) unchanged.
In `@Example/Windows/SolidSyslogWindowsExample.c`:
- Around line 157-171: Check the return value from _beginthreadex when assigning
to serviceThread (it returns 0 on failure, not INVALID_HANDLE_VALUE); if
_beginthreadex returns 0, set shutdownFlag=true, perform any necessary cleanup
and fail fast instead of calling ExampleInteractive_Run. Specifically, after
calling _beginthreadex that creates serviceThread for ServiceThreadEntry, verify
serviceThread != 0, and on failure avoid calling ExampleInteractive_Run with an
invalid handle and skip WaitForSingleObject/CloseHandle; log/report the error
and cleanup resources before returning.
🪄 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: 7c8e4cdc-a727-476d-ae4a-02deee4eb3cb
📒 Files selected for processing (11)
Bdd/features/buffered.featureBdd/features/steps/syslog_steps.pyBdd/features/tcp_singletask.featureBdd/features/tcp_transport.featureDEVLOG.mdExample/CMakeLists.txtExample/Windows/SolidSyslogWindowsExample.cREADME.mdSKILL.mddocs/bdd.mddocs/iec62443.md
The pre-S13.18 run_example wrote `send N\nquit\n` upfront via process.communicate() and relied on NullBuffer's synchronous Send to guarantee delivery before exit. After S13.18 the Windows SolidSyslogExample.exe is buffered (CircularBuffer + service thread); `quit` could land before the service thread had drained, losing the UDP packet. Effect on the first push: 9 features that use "the example program sends..." regressed on Windows (header_fields, message_fields, origin, prival, structured_data, syslog, time_quality, timestamp, udp_mtu) — all reporting "oracle received 0 of 1 messages within 5 seconds". Same fix as the cross-platform `the buffered example` step from the previous slice: route through _run_with_prompt_protocol so the oracle confirms receipt before `quit`. The Linux SingleTask binary supports the prompt protocol identically — its NullBuffer just makes the Sent N message print immediately, so wait_for_messages returns instantly. Verified: Linux 12 features × 30 scenarios pass against syslog-ng (syslog, header_fields, timestamp, structured_data, time_quality, origin, prival, message_fields, udp_mtu, buffered, tcp_singletask, tcp_transport).
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1049 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
`SolidSyslogNullBuffer` performs the transport Send on the caller's
thread, so SolidSyslog_Log() with NullBuffer wired is synchronous —
bounded by SO_SNDTIMEO but blocking until the bytes leave the socket.
The non-blocking property only holds in buffered wirings
(CircularBuffer / PosixMessageQueueBuffer) where Service() drains
on a separate thread.
The pre-S13.18 row had the same fudge ("NullBuffer for immediate
send" buried in a non-blocking claim); rewriting the cell for S13.18
was the moment to correct it but I perpetuated it. CodeRabbit caught
on the PR review.
New SR 6.1 row makes the trade-off explicit: two wirings, two cost
models, caller picks per deployment.
|
On the Docstring Coverage 52.94% < 80% pre-merge warning — leaving as-is, deliberately. The new helpers added or rewritten in this PR all have docstrings explaining the why, not just the what: The 52.94% gap is almost entirely Behave step-decorator functions ( Adding throwaway docstrings to ~20 step functions just to hit the threshold would be noise — and it would obscure the fact that the helper functions, which actually need the why, do have substantive ones. |
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1049 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
1 participant
Closes #244.
Wires the Windows BDD runner to exercise the buffering path end-to-end via the new portable ring buffer (
SolidSyslogCircularBuffer, S04.05). The cross-platform UDP and TCP buffered scenarios (buffered.feature,tcp_transport.feature) now run on bothbdd-linux-syslog-ngandbdd-windows-otel.Slices
test: portable wait_for_prompt for cross-platform BDD— replaceselect.select+os.readin the Python step layer with a daemon-thread +queue.Queuereader. Same Python signature; works on POSIX and Windows pipe fds. Picked over a production drain-on-shutdown contract so the library doesn't bake test convenience into shipped semantics, and so ungraceful shutdown stays a real testable scenario.feat: Windows example uses CircularBuffer + service thread— replaceSolidSyslogNullBuffer_Create(sender)with caller-allocatedSolidSyslogCircularBufferstorage backed bySolidSyslogWindowsMutex. Add a Win32 service thread launched via_beginthreadexdriving the existingCommon/ExampleServiceThread.c(now linked into the Windows binary as well as Linux Threaded).shutdownFlag+WaitForSingleObjectmirror the Linuxpthread_join.<windows.h>pinned after<winsock2.h>with a comment so a future cleanup can't alphabetise it back.test: enable buffered + tcp_transport on the Windows BDD runner— drop@bufferedfrombuffered.featureandtcp_transport.feature; rename their step text tothe buffered example…. Step layer routes the new prose toTHREADED_BINARYon the Linux oracle andcontext.example_binaryon the Windows oracle. Other@bufferedfeatures (file store, switching, mtls/tls, tcp_reconnect) keep their tag and prose — they really are pthread/syslog-ng-specific.tcp_singletask.featuretagged@windows_wip(Linux-only now) sincetcp_transportcovers Windows TCP via the buffered example.docs: update README/SKILL/bdd/iec62443 + DEVLOG— README/SKILL drop "planned" wording on CircularBuffer and update the Windows example description.docs/bdd.mdrewrites the@bufferedtag meaning.docs/iec62443.mdupdates SR 6.1, the bare-metal/RTOS deployment recipe, and the SL1 starter components to position CircularBuffer as the portable threaded buffer. DEVLOG records decisions, deferred items, and open questions.Out of scope (deferred)
SolidSyslogPosixMessageQueueBufferfrom the Linux Threaded example — explicitly scoped out by the user.CreateSender/DestroySenderfactories intoExample/Common/— the shapes diverge too much (SwitchingSender vs single-sender) without first widening the Windows example. Flagged for a follow-up refactor.SolidSyslogSwitchingSender, file-backedSolidSyslogBlockStore, or TLS to the Windows example — those are S13.19 / later epic-13 stories.ExampleServiceThread_Runand aSolidSyslogBuffer.HasPendingprobe — deliberate API decision not taken in this story.ExampleInteractive_Run's stdin handling against a leading UTF-8 BOM (.NET StreamWriter quirk; the BDD harness uses Python which doesn't emit one, so not a CI hazard).Acceptance
bdd-windows-otelrunsBdd/features/buffered.feature(2 scenarios) andBdd/features/tcp_transport.feature(1 scenario), all green.bdd-linux-syslog-ngcontinues green — both features still work via the Linux Threaded binary.@bufferedfeatures remain excluded on Windows via tags and stay green on Linux.NullBufferreference left inExample/Windows/.Local validation
msvc-debugbuild clean; Linuxdebugexamples (SolidSyslogExample,SolidSyslogThreadedExample) still build cleanly.clang-format+IWYU(clang devcontainer) green.buffered,tcp_transport,switching_transport,mtls_transport,tls_transport,block_lifecycle).not @wip: 16/16 features pass (5 errors are the pre-existingsyslog-ng.confchmod issue on Windows-host bind mounts, unrelated to this PR).send→Sent 1 message+ prompt → UDP datagram arrives on127.0.0.1:5514with proper RFC 5424 framing →quit→ exit 0.Test plan
bdd-windows-otelCI job passes (the new acceptance signal).bdd-linux-syslog-ngCI job passes.build-linux-gcc,build-linux-clang,build-windows-msvc,sanitize-linux-gcc,coverage-linux-gcc,analyze-tidy,analyze-cppcheck,analyze-format,analyze-iwyu,integration-linux-openssl) green.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation