feat: S12.14 decouple buffer drain from sender state — fail-fast streams + eager Service drain#283
Conversation
… state ProcessMessages now drains the buffer eagerly into the store and then attempts one send from the store. When a write does not land in the store (NullStore, or store-write rejection), it falls back to a best-effort direct send to preserve the existing single-task / no-store-and-forward configurations. StoreFake promoted to FIFO (8 slots) with a write counter for the burst-drain assertion. New Service tests use the production CircularBuffer + NullMutex.
Stream becomes fully non-blocking from socket creation. connect() returns EINPROGRESS and the wait is bounded via select() to 200 ms (mirrors the Winsock pattern); SO_ERROR distinguishes completed-success from deferred- failure. Send is single-call, fail-fast, with internal close on any failure (short write, EAGAIN, errno). Read returns 0 on would-block and -1 with internal close on EOF/error. SolidSyslogStream.h documents the new contract. Drops SO_SNDTIMEO (no-op on non-blocking sockets) and the EINTR retry loop. SocketFake gains fcntl, select, connect-with-errno, recv-with-errno, and getsockopt(SO_ERROR) seams to drive the new code paths under test.
The socket is now non-blocking from creation through the full lifecycle — no longer flips back to blocking after a successful connect. Send returns true only when the kernel accepts the entire frame in one call; any failure (short write, WSAEWOULDBLOCK, error) closes the socket internally and returns false. Read returns 0 on WSAEWOULDBLOCK and -1 with internal close on EOF (recv == 0) or other errors. Drops SO_SNDTIMEO (no-op on non-blocking sockets). WinsockFake gains FailNextRecvWithLastError so tests can drive the would-block / error / EOF Read paths. Validated locally on MSVC: 956 tests pass, including the 50 Winsock TCP stream tests that exercise the new non-blocking Send / Read contract.
Five coordinated changes so TLS works on top of the now-non-blocking PosixTcp/WinsockTcp streams: - BIO read translates would-block (0) into BIO_set_retry_read + return -1, and clears retry on EOF/error. Without this, OpenSSL would treat the transport's would-block as EOF and abort the handshake on first poll. - BIO write clears retry flags on transport-Send failure so SSL_write surfaces SSL_ERROR_SYSCALL rather than spinning on a closed transport. - PerformHandshake drives SSL_connect in a bounded retry loop with a 5 s budget and a 1 ms poll interval — sleeps on WANT_READ / WANT_WRITE, fails fast on any hard SSL error. - Send is single-call, fail-fast: SSL_write != size closes the SSL session and the underlying transport so the StreamSender's reconnect path picks up on the next service tick. - Read returns 0 on WANT_READ (mirrors transport contract); anything else (errors, peer close, mid-stream renegotiation needing WANT_WRITE) takes the close + -1 path. Comment in TlsStream_Read records the handshake- vs-steady-state distinction explicitly. - TlsStream_Close is now idempotent so internal-close-from-Send/Read followed by an explicit Close from the StreamSender doesn't double-free. OpenSslFake gains SSL_connect return-sequence injection, SSL_get_error, SSL_write/read return overrides, and BIO_set_flags / BIO_clear_flags spies so the new code paths are covered without touching the real OpenSSL during unit tests. TlsStream_sleep is a function-pointer seam (Windows Sleep / POSIX nanosleep by default; tests UT_PTR_SET to a no-op). Coverage 100%. Validated locally on MSVC: 973 tests pass.
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits 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)
📝 WalkthroughWalkthroughThis PR implements S12.14, which decouples buffer drain from sender state via two coordinated changes: streams become non-blocking/fail-fast (closing on any I/O failure), and ProcessMessages switches to eager buffer drain (pump all buffer→store, then attempt one store send). This fixes a critical bug where a slow sender would stall the buffer→store pump, causing producer buffer drops before the store (with its overflow policy) could engage. The change introduces a sleep abstraction for bounded TLS handshake retries and significantly expands test infrastructure (SocketFake, OpenSslFake, StoreFake, WinsockFake) to validate non-blocking contracts across all three stream implementations. ChangesNon-blocking Stream Refactor & Eager Buffer Drain
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
|
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1072 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
POSIX fcntl(int fd, int cmd, ...) has two adjacent int parameters by contract; we cannot change the signature in the link-substituted fake. NOLINTNEXTLINE on the existing readability-inconsistent suppression.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
Tests/StoreFake.c (1)
76-86: 💤 Low valueLGTM — minor note on silent-drop semantics when queue is full.
When
count >= STOREFAKE_MAX_MESSAGES,Write()drops the payload silently but still incrementswriteCountand returnstrue. This means future callers ofHasUnsent()will not see the overflow message, andStoreDidNotRetainLastWrite()(in the current production code — see separate comment) would incorrectly conclude the write was retained. Since this is a test fake with a queue depth of 8, it will not affect current tests, but the semantics diverge from a real store.No immediate action required; just worth noting if the fake is ever stress-tested with > 8 in-flight messages.
🤖 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 `@Tests/StoreFake.c` around lines 76 - 86, The fake store's Write() currently drops payloads when fake->count >= STOREFAKE_MAX_MESSAGES but still increments fake->writeCount and returns true, diverging from real-store semantics; change Write() (the function handling fake->count, fake->writeCount and memcpy into fake->entries[]) so that when the queue is full it does not increment fake->writeCount and returns false (or otherwise signals failure), ensuring HasUnsent() and tests like StoreDidNotRetainLastWrite() observe the dropped write consistently; update any callers/tests expecting the previous silent-accept behavior if needed.Tests/SolidSyslogTest.cpp (1)
1476-1531: ⚡ Quick winExtract shared CircularBuffer + StoreFake + NullMutex fixture into a
TEST_GROUP_BASE.Both new tests duplicate the same setup/teardown sequence (create circular buffer, create store, configure service, restore original config, destroy all). Per coding guidelines,
TEST_BASE / TEST_GROUP_BASEshould be used when multipleTEST_GROUPs (or multiple tests within a group) declare the same storage/device variables and setup/teardown.As per coding guidelines: "Use
TEST_BASE / TEST_GROUP_BASEfor shared fixture when multipleTEST_GROUPs declare same storage/file/device variables and setup/teardown."🤖 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 `@Tests/SolidSyslogTest.cpp` around lines 1476 - 1531, Both tests duplicate circular buffer + store + null mutex setup/teardown; extract that shared fixture into a TEST_GROUP_BASE and have the two tests use it: move the static bufferStorage, circularBuffer (created via SolidSyslogCircularBuffer_Create), fakeStore (StoreFake_Create), and null mutex (SolidSyslogNullMutex_Create) plus serviceConfig/SolidSyslog_Create call into the TEST_GROUP_BASE setup, and move SolidSyslog_Destroy(), SolidSyslog_Create(&config), StoreFake_Destroy(), SolidSyslogCircularBuffer_Destroy() and SolidSyslogNullMutex_Destroy() into the TEST_GROUP_BASE teardown; update the tests ServiceEagerlyDrainsBufferIntoStoreInOneTick and ServiceSendsStoredMessagesInFifoOrderAcrossTicks to remove their duplicated setup/teardown and rely on the base fixture while keeping their specific SenderFake calls and assertions.Tests/Support/SocketFake.h (1)
94-100: 💤 Low value
exceptfdswording may mislead: non-blocking connect errors appear inwritefds, not onlyexceptfds.The comment says
hasError=true sets fd in exceptfds, but on Linux (and most POSIX systems) a failed non-blocking connect sets the fd as writable (inwritefds) and the actual error is retrieved viagetsockopt(SO_ERROR).exceptfdsis for out-of-band data. If the production connect-completion path only checkswritefds(which is the correct approach), a test usingSetSelectError(true)withoutSetSelectWritable(true)would not exercise the right code path. Consider clarifying whether this simulates:
- a
select()syscall failure (returns -1), or- a writable-but-errored fd (fd set in
writefds, SO_ERROR ≠ 0).📝 Suggested comment clarification
- /* select configuration. ready=true makes select report fd writable in writefds; - ready=false leaves writefds empty (timeout). hasError=true sets fd in - exceptfds. returnValue overrides the int return (1 ready, 0 timeout, -1 error). - Default: returns 1 (one fd ready: writable, no error). */ + /* select configuration. ready=true makes select report fd writable in writefds; + ready=false leaves writefds empty (timeout). hasError=true sets the fd in + exceptfds (out-of-band / error indicator); to model a failed non-blocking + connect, combine SetSelectWritable(true) + SetSoError(ECONNREFUSED) instead. + returnValue overrides the int return (1 ready, 0 timeout, -1 error). + Default: returns 1 (one fd ready: writable, no error). */🤖 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 `@Tests/Support/SocketFake.h` around lines 94 - 100, The comment describing select simulation is misleading: SocketFake_SetSelectError(bool hasError) should not imply errors are signalled via exceptfds only; on POSIX a non-blocking connect failure is indicated by the fd being writable (writefds) with SO_ERROR set. Update the comment to state the two distinct simulations supported (a select() failure via SocketFake_SetSelectReturn(-1) vs. a connect-completion-with-error simulated by setting writefds and a non-zero SO_ERROR when SocketFake_SetSelectError(true)), and document that SocketFake_SetSelectWritable(true) models the fd appearing in writefds and that the test harness must call both SetSelectWritable(true) and SetSelectError(true) to simulate a writable-but-errored socket (getsockopt(SO_ERROR) non-zero).
🤖 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 `@Core/Interface/SolidSyslogStream.h`:
- Around line 31-36: The doc for SolidSyslogStream_Read is incomplete: when it
returns < 0 it closes the underlying socket internally and the caller must call
Open before attempting either the next Send or the next Read; update the comment
above SolidSyslogStream_Read to mention that the caller must call Open before
the next Read as well as before the next Send (referencing
SolidSyslogStream_Read, Open, and Send to locate the text to change).
In `@Core/Source/SolidSyslog.c`:
- Around line 137-156: DrainBufferIntoStore currently decides to fallback to
direct send using StoreDidNotRetainLastWrite(), which checks
SolidSyslogStore_HasUnsent() and can hide a failed write when the store already
had unsent messages; instead, capture and use the return value of
SolidSyslogStore_Write(...) directly (e.g. bool wrote =
SolidSyslogStore_Write(...)) and call SolidSyslogSender_Send(...) when wrote is
false to implement the intended "write rejected -> best-effort direct send"
behavior; also remove the now-unused StoreDidNotRetainLastWrite helper (and its
forward declaration) from the translation unit.
In `@Tests/SolidSyslogTest.cpp`:
- Around line 1478-1480: The test allocates SolidSyslogCircularBufferStorage
bufferStorage[...] on the stack which can be freed if a test assertion fails and
teardown runs (leading to use-after-stack when SolidSyslog_Destroy accesses
circularBuffer); change the declaration to static so the storage has program
lifetime. Update both occurrences where SolidSyslogCircularBufferStorage
bufferStorage[...] and the corresponding SolidSyslogCircularBuffer_Create call
(symbols: SolidSyslogCircularBufferStorage, bufferStorage,
SOLIDSYSLOG_CIRCULARBUFFER_STORAGE_SIZE_BYTES, SolidSyslogCircularBuffer_Create,
circularBuffer) are used so teardown/SolidSyslog_Destroy sees valid storage even
after stack unwinding. Ensure no other initialization changes are needed because
SolidSyslogCircularBuffer_Create re-initializes the storage.
---
Nitpick comments:
In `@Tests/SolidSyslogTest.cpp`:
- Around line 1476-1531: Both tests duplicate circular buffer + store + null
mutex setup/teardown; extract that shared fixture into a TEST_GROUP_BASE and
have the two tests use it: move the static bufferStorage, circularBuffer
(created via SolidSyslogCircularBuffer_Create), fakeStore (StoreFake_Create),
and null mutex (SolidSyslogNullMutex_Create) plus
serviceConfig/SolidSyslog_Create call into the TEST_GROUP_BASE setup, and move
SolidSyslog_Destroy(), SolidSyslog_Create(&config), StoreFake_Destroy(),
SolidSyslogCircularBuffer_Destroy() and SolidSyslogNullMutex_Destroy() into the
TEST_GROUP_BASE teardown; update the tests
ServiceEagerlyDrainsBufferIntoStoreInOneTick and
ServiceSendsStoredMessagesInFifoOrderAcrossTicks to remove their duplicated
setup/teardown and rely on the base fixture while keeping their specific
SenderFake calls and assertions.
In `@Tests/StoreFake.c`:
- Around line 76-86: The fake store's Write() currently drops payloads when
fake->count >= STOREFAKE_MAX_MESSAGES but still increments fake->writeCount and
returns true, diverging from real-store semantics; change Write() (the function
handling fake->count, fake->writeCount and memcpy into fake->entries[]) so that
when the queue is full it does not increment fake->writeCount and returns false
(or otherwise signals failure), ensuring HasUnsent() and tests like
StoreDidNotRetainLastWrite() observe the dropped write consistently; update any
callers/tests expecting the previous silent-accept behavior if needed.
In `@Tests/Support/SocketFake.h`:
- Around line 94-100: The comment describing select simulation is misleading:
SocketFake_SetSelectError(bool hasError) should not imply errors are signalled
via exceptfds only; on POSIX a non-blocking connect failure is indicated by the
fd being writable (writefds) with SO_ERROR set. Update the comment to state the
two distinct simulations supported (a select() failure via
SocketFake_SetSelectReturn(-1) vs. a connect-completion-with-error simulated by
setting writefds and a non-zero SO_ERROR when SocketFake_SetSelectError(true)),
and document that SocketFake_SetSelectWritable(true) models the fd appearing in
writefds and that the test harness must call both SetSelectWritable(true) and
SetSelectError(true) to simulate a writable-but-errored socket
(getsockopt(SO_ERROR) non-zero).
🪄 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: 013a7935-83b6-460f-a7d8-6e45c8b2c18c
📒 Files selected for processing (24)
Core/Interface/SolidSyslogStream.hCore/Source/SolidSyslog.cDEVLOG.mdPlatform/OpenSsl/Source/SolidSyslogTlsStream.cPlatform/OpenSsl/Source/SolidSyslogTlsStreamInternal.hPlatform/Posix/Source/SolidSyslogPosixTcpStream.cPlatform/Windows/Source/SolidSyslogWinsockTcpStream.cTests/CMakeLists.txtTests/SolidSyslogPosixTcpStreamTest.cppTests/SolidSyslogStreamSenderTest.cppTests/SolidSyslogTest.cppTests/SolidSyslogTlsStreamTest.cppTests/SolidSyslogWinsockTcpStreamTest.cppTests/StoreFake.cTests/StoreFake.hTests/StoreFakeTest.cppTests/StreamFake.cTests/StreamFake.hTests/Support/OpenSslFake.cTests/Support/OpenSslFake.hTests/Support/SocketFake.cTests/Support/SocketFake.hTests/Support/WinsockFake.cTests/Support/WinsockFake.h
👮 Files not reviewed due to content moderation or server errors (7)
- Platform/Posix/Source/SolidSyslogPosixTcpStream.c
- Platform/Windows/Source/SolidSyslogWinsockTcpStream.c
- DEVLOG.md
- Tests/SolidSyslogPosixTcpStreamTest.cpp
- Platform/OpenSsl/Source/SolidSyslogTlsStream.c
- Tests/SolidSyslogTlsStreamTest.cpp
- Tests/Support/SocketFake.c
…pFunction SKILL.md bans #ifdef in production code. The TlsStream's bounded handshake retry sleep is now an integrator-injected callback (SolidSyslogSleepFunction typedef in Core/Interface/SolidSyslogSleep.h). Two platform implementations ship alongside the rest of the platform-specific helpers: - SolidSyslogPosixSleep wraps nanosleep. - SolidSyslogWindowsSleep wraps Sleep. The example TLS-sender wirings inject the platform impl (PosixSleep on the Linux Threaded example, WindowsSleep on the Windows example). Unit tests inject a local NoOpSleep through the config field instead of UT_PTR_SET on a function-pointer seam, so SolidSyslogTlsStreamInternal.h is no longer needed and is deleted. SOLIDSYSLOG_TLS_STREAM_SIZE bumped from 13 to 14 intptr_t slots to account for the new config field.
- SolidSyslogStream.h: Read doc now mentions "call Open before next Send or Read" (the < 0 path also closes the socket internally so a follow-up Read needs Open too). - SolidSyslog.c DrainBufferIntoStore: switch to using Store_Write's return value directly. The previous StoreDidNotRetainLastWrite() helper proxied via !HasUnsent(), which silently dropped a rejected Write into a non-empty store (e.g. BlockStore full + HALT discard policy). The bool contract is now explicit: true = retained for later replay; false = not held by this store. - SolidSyslogNullStore.c Write now returns false to honour that contract — NullStore never retains, so the eager-drain loop's !Write() branch takes the direct-send path. Preserves the constrained-system real-buffer + NullStore + UDP "one attempt per message, no buffering" wiring used by the Threaded example's --store=null mode. - StoreFake.c Write: queue-full now returns false and skips the writeCount bump so HasUnsent() and the test counters stay consistent with what was actually retained — mirrors the new real-store semantics. - SolidSyslogTest.cpp: lift the duplicated CircularBuffer + StoreFake + NullMutex setup out of the two new eager-drain tests into a SolidSyslogServiceEagerDrain TEST_GROUP. bufferStorage moves inside setup() with static storage duration so a CHECK failure that skips the test body's cleanup cannot leave a dangling stack reference behind for SolidSyslog_Destroy in teardown. - SocketFake.h: clarify the select() seam comment to spell out the three distinct simulations the fake supports — successful connect, deferred connect failure (writefds + SO_ERROR), and exceptfds rejection.
ReadNextUnsent already returns false on an empty store (NullStore and the real BlockStore both honour the contract), so the leading HasUnsent check in the && chain is dead weight. The remaining ReadNextUnsent && Send → MarkSent reads more directly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The literal 1 in WinsockTcpStream::WaitForConnectCompletion's select() call relied on a nearby comment for meaning. Lifting it to a named WINSOCK_NFDS_IGNORED enum constant moves the explanation to the definition site and makes the call read at one level of abstraction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ments The eager-drain TEST_GROUP used a 9-element positional brace initializer for SolidSyslogConfig — silently shifts if a new field is added to the config struct. Switch to value-init plus explicit named assignments so new fields default to zero/nullptr and the intent of each populated slot is visible at the call site. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The non-blocking BIO, handshake retry, Send fail-fast, and Read non-blocking test groups added in S12.14 each repeated a 4-6 line arrangement: open + drive a fake's return + invoke the SUT + grab a callback pointer. Lift the patterns into TEST_GROUP member helpers (InvokeBioRead*, ArrangeHandshake*, OpenThenCauseSslWriteFailure, SendShortMessage, OpenThenReadWithSslReturnAndError) so each test reads as one or two sentences. Same treatment for repeated assertion shapes — five CHECK_* macros (CHECK_BIO_READ_RETRY_SIGNALLED, CHECK_BIO_RETRY_FLAGS_CLEARED, CHECK_SSL_SESSION_CLOSED, CHECK_TRANSPORT_CLOSED_ONCE etc.) name the intent of each multi-line check so the failing-line pointer still maps to the caller, mirroring the CHECK_PRIVAL convention from SolidSyslogTest.cpp. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The non-blocking-Read tests added in S12.14 each declared a 16-byte buffer and called SolidSyslogStream_Read inline; the close-fd assertions captured an openFd local before invoking the path that closes it and then asserted (count == 1, lastClosed == openFd). Lift each into the TEST_GROUP / file-local form: a Read16ByteBuffer member returning the ssize directly, and a CHECK_SOCKET_CLOSED_ONCE macro that pairs the count check with the matching-fd check. Same pattern applied to both the POSIX and Winsock test files for symmetry. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1090 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
IWYU reported missing forward declarations for the two test bodies that reference the new CHECK_TRANSPORT_CLOSED_ONCE macro (ReadReturnsNegativeOneOnHardErrorAndClosesSsl, SendClosesTransportOnWriteFailure). Same forward-decl pattern as other test files in the repo. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1090 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
Tests/SolidSyslogTlsStreamTest.cpp (1)
15-22: 💤 Low value
g_lastSleepMsis captured but never asserted.
NoOpSleeprecordsg_lastSleepMson every call, but no test reads it. Either drop it (it's dead test plumbing) or use it to lock down the documentedHANDSHAKE_POLL_INTERVAL_MILLISECONDS=1pacing — a one-lineLONGS_EQUAL(1, g_lastSleepMs);inOpenSleepsBetweenHandshakeRetriesprevents an accidental constant change from going unnoticed.🪶 Suggested addition (preferred)
TEST(SolidSyslogTlsStream, OpenSleepsBetweenHandshakeRetries) { ArrangeHandshakeRetryThenSucceed(SSL_ERROR_WANT_READ); SolidSyslogStream_Open(stream, addr); LONGS_EQUAL(1, g_sleepCallCount); + LONGS_EQUAL(1, g_lastSleepMs); /* HANDSHAKE_POLL_INTERVAL_MILLISECONDS */ }🤖 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 `@Tests/SolidSyslogTlsStreamTest.cpp` around lines 15 - 22, g_lastSleepMs is recorded by NoOpSleep but never asserted; update the test OpenSleepsBetweenHandshakeRetries to assert the expected pacing by adding a check that g_lastSleepMs equals HANDSHAKE_POLL_INTERVAL_MILLISECONDS (e.g., LONGS_EQUAL(HANDSHAKE_POLL_INTERVAL_MILLISECONDS, g_lastSleepMs)) after the Open call completes, or remove g_lastSleepMs and its assignment from NoOpSleep if you prefer to drop the dead plumbing; modify either NoOpSleep or OpenSleepsBetweenHandshakeRetries accordingly to keep test plumbing and expectations consistent.
🤖 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 `@Platform/OpenSsl/Interface/SolidSyslogTlsStream.h`:
- Line 7: Update the callback API to include a context pointer and expose it in
the TLS stream config: change the typedef SolidSyslogSleepFunction in
Core/Interface/SolidSyslogSleep.h from void (*)(int milliseconds) to void
(*)(void* context, int milliseconds), add a corresponding void* sleepContext
field to SolidSyslogTlsStreamConfig in SolidSyslogTlsStream.h, and then update
all implementations and call sites (e.g., SolidSyslogPosixSleep,
SolidSyslogWindowsSleep and any callers) to accept the new (void* context, int
milliseconds) signature and pass the config->sleepContext when invoking the
sleep callback. Ensure the config field name exactly matches sleepContext so
tests and instrumentation can supply the context.
In `@Platform/OpenSsl/Source/SolidSyslogTlsStream.c`:
- Around line 340-369: Update the sleep callback to accept a context pointer:
change the SolidSyslogSleepFunction typedef to take (int milliseconds, void*
context), add a void* sleepContext field to SolidSyslogTlsStreamConfig, and
update all call sites (e.g., TlsStream_PerformHandshake) to call
stream->config.sleep(HANDSHAKE_POLL_INTERVAL_MILLISECONDS,
stream->config.sleepContext); ensure any assignments to config.sleep also match
the new signature.
- Line 363: SolidSyslogTlsStream_Create must validate that the provided config
and its required callback sleep are non-NULL to avoid dereferencing
stream->config.sleep during handshake retries; modify
SolidSyslogTlsStream_Create to check that config != NULL and config->sleep !=
NULL (or stream->config.sleep after copying) and if missing return NULL (and
optionally log an error) so the function fails fast instead of allowing the
service thread to crash when HANDSHAKE_POLL_INTERVAL_MILLISECONDS is used during
WANT_READ/WANT_WRITE retries.
In `@Tests/SolidSyslogTlsStreamTest.cpp`:
- Around line 124-136: The four single-statement CHECK_* macros
(CHECK_BIO_READ_RETRY_SIGNALLED, CHECK_BIO_READ_RETRY_NOT_SIGNALLED,
CHECK_BIO_RETRY_FLAGS_CLEARED, CHECK_TRANSPORT_CLOSED_ONCE) violate the required
do { ... } while (0) pattern; update each macro to wrap its assertion in a do {
... } while (0) block (matching the style used by CHECK_SSL_SESSION_CLOSED) so
they remain safe if later expanded, keeping the existing // NOLINTBEGIN/ //
NOLINTEND guards intact.
- Around line 1052-1057: The test
SolidSyslogTlsStream::ReadReturnsNegativeOneOnZeroReturnAndClosesSsl currently
verifies only that the SSL session is closed after an SSL_ERROR_ZERO_RETURN but
not that the underlying transport was closed; update the test (the call to
OpenThenReadWithSslReturnAndError(0, SSL_ERROR_ZERO_RETURN)) to also assert
transport closure by adding CHECK_TRANSPORT_CLOSED_ONCE() after
CHECK_SSL_SESSION_CLOSED(), ensuring TlsStream_Close’s behavior of
unconditionally closing the transport is validated for the clean-shutdown path.
---
Nitpick comments:
In `@Tests/SolidSyslogTlsStreamTest.cpp`:
- Around line 15-22: g_lastSleepMs is recorded by NoOpSleep but never asserted;
update the test OpenSleepsBetweenHandshakeRetries to assert the expected pacing
by adding a check that g_lastSleepMs equals HANDSHAKE_POLL_INTERVAL_MILLISECONDS
(e.g., LONGS_EQUAL(HANDSHAKE_POLL_INTERVAL_MILLISECONDS, g_lastSleepMs)) after
the Open call completes, or remove g_lastSleepMs and its assignment from
NoOpSleep if you prefer to drop the dead plumbing; modify either NoOpSleep or
OpenSleepsBetweenHandshakeRetries accordingly to keep test plumbing and
expectations consistent.
🪄 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: 1d4d34ec-79e9-40a2-9214-328f1544f984
📒 Files selected for processing (28)
CLAUDE.mdCore/Interface/SolidSyslogSleep.hCore/Interface/SolidSyslogStream.hCore/Source/SolidSyslog.cCore/Source/SolidSyslogNullStore.cExample/Common/ExampleTlsSender_OpenSsl_PosixTcp.cExample/Common/ExampleTlsSender_OpenSsl_WinsockTcp.cPlatform/OpenSsl/Interface/SolidSyslogTlsStream.hPlatform/OpenSsl/Source/SolidSyslogTlsStream.cPlatform/Posix/CMakeLists.txtPlatform/Posix/Interface/SolidSyslogPosixSleep.hPlatform/Posix/Source/SolidSyslogPosixSleep.cPlatform/Windows/CMakeLists.txtPlatform/Windows/Interface/SolidSyslogWindowsSleep.hPlatform/Windows/Source/SolidSyslogWindowsSleep.cPlatform/Windows/Source/SolidSyslogWinsockTcpStream.cTests/CMakeLists.txtTests/OpenSslIntegration/SolidSyslogTlsStreamIntegrationTest.cppTests/SolidSyslogNullStoreTest.cppTests/SolidSyslogPosixSleepTest.cppTests/SolidSyslogPosixTcpStreamTest.cppTests/SolidSyslogTest.cppTests/SolidSyslogTlsStreamTest.cppTests/SolidSyslogWindowsSleepTest.cppTests/SolidSyslogWinsockTcpStreamTest.cppTests/StoreFake.cTests/Support/SocketFake.cTests/Support/SocketFake.h
✅ Files skipped from review due to trivial changes (7)
- Platform/Windows/Source/SolidSyslogWindowsSleep.c
- Platform/Posix/Interface/SolidSyslogPosixSleep.h
- Core/Interface/SolidSyslogSleep.h
- Platform/Posix/Source/SolidSyslogPosixSleep.c
- Tests/SolidSyslogPosixSleepTest.cpp
- Platform/Windows/Interface/SolidSyslogWindowsSleep.h
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (8)
- Core/Interface/SolidSyslogStream.h
- Core/Source/SolidSyslog.c
- Tests/SolidSyslogWinsockTcpStreamTest.cpp
- Tests/StoreFake.c
- Tests/SolidSyslogPosixTcpStreamTest.cpp
- Tests/SolidSyslogTest.cpp
- Tests/Support/SocketFake.c
- Tests/Support/SocketFake.h
| #include <stdint.h> | ||
|
|
||
| #include "ExternC.h" | ||
| #include "SolidSyslogSleep.h" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the current SolidSyslogSleepFunction typedef and all call sites
# to verify the context parameter is absent everywhere.
rg -n "SolidSyslogSleepFunction" --type=c --type=h --type=cpp -A2Repository: DavidCozens/solid-syslog
Length of output: 1482
SolidSyslogSleepFunction violates the callback guideline — missing void* context parameter and paired config field.
The project rule for new callbacks requires:
For new callbacks, the function pointer must take a
void* contextparameter. Config struct must expose a paired context field.
SolidSyslogSleepFunction is defined in Core/Interface/SolidSyslogSleep.h as void (*)(int milliseconds) with no context parameter. Both platform implementations (SolidSyslogPosixSleep and SolidSyslogWindowsSleep) follow this signature. SolidSyslogTlsStreamConfig in this file exposes no matching sleepContext field. This diverges from the callback API standard and makes test-observable instrumentation needlessly awkward.
Fix requires changes in:
Core/Interface/SolidSyslogSleep.h: addvoid* contextto typedefSolidSyslogTlsStream.h: addsleepContextfield to config struct- All implementations and call sites: update signatures and pass context through
♻️ Proposed change for SolidSyslogTlsStream.h
struct SolidSyslogTlsStreamConfig
{
struct SolidSyslogStream* transport;
SolidSyslogSleepFunction sleep; /* drives bounded handshake retry ... — required */
+ void* sleepContext; /* passed verbatim to sleep() — may be NULL if callback ignores it */
const char* caBundlePath;🤖 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 `@Platform/OpenSsl/Interface/SolidSyslogTlsStream.h` at line 7, Update the
callback API to include a context pointer and expose it in the TLS stream
config: change the typedef SolidSyslogSleepFunction in
Core/Interface/SolidSyslogSleep.h from void (*)(int milliseconds) to void
(*)(void* context, int milliseconds), add a corresponding void* sleepContext
field to SolidSyslogTlsStreamConfig in SolidSyslogTlsStream.h, and then update
all implementations and call sites (e.g., SolidSyslogPosixSleep,
SolidSyslogWindowsSleep and any callers) to accept the new (void* context, int
milliseconds) signature and pass the config->sleepContext when invoking the
sleep callback. Ensure the config field name exactly matches sleepContext so
tests and instrumentation can supply the context.
There was a problem hiding this comment.
Declined — sleep is stateless. Both platform implementations (nanosleep / Sleep) are pure functions over int milliseconds with no instance state to carry, so a void* context parameter would always be passed NULL at the call site. The CLAUDE.md rule for new callbacks covers cases where context-passing is a real need; a thin platform-API wrapper is not one.
There was a problem hiding this comment.
@DavidCozens, understood — that's a reasonable distinction. A thin wrapper over a stateless OS primitive (nanosleep / Sleep) doesn't need a context slot, and mandating one would only add noise at every call site. I'll note this nuance for future reviews.
✏️ Learnings added
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 283
File: Platform/OpenSsl/Interface/SolidSyslogTlsStream.h:7-7
Timestamp: 2026-05-07T08:22:53.110Z
Learning: In the `solid-syslog` project, the CLAUDE.md rule requiring a `void* context` parameter (and paired config field) for new callbacks applies only to callbacks that carry real instance state. Thin wrappers over stateless OS primitives — such as `SolidSyslogSleepFunction` (wrapping `nanosleep`/`Sleep`) — are exempt: both platform implementations are pure functions over `int milliseconds` with no state to carry, so a context pointer would always be NULL and should not be added.
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.
| static inline bool TlsStream_PerformHandshake(struct SolidSyslogTlsStream* stream) | ||
| { | ||
| return SSL_connect(stream->ssl) > 0; | ||
| int totalSleptMs = 0; | ||
| bool result = false; | ||
| bool done = false; | ||
|
|
||
| while (!done) | ||
| { | ||
| int rc = SSL_connect(stream->ssl); | ||
| if (rc > 0) | ||
| { | ||
| result = true; | ||
| done = true; | ||
| } | ||
| else | ||
| { | ||
| int err = SSL_get_error(stream->ssl, rc); | ||
| if (!IsRetryableSslError(err) || IsHandshakeBudgetExhausted(totalSleptMs)) | ||
| { | ||
| done = true; | ||
| } | ||
| else | ||
| { | ||
| stream->config.sleep(HANDSHAKE_POLL_INTERVAL_MILLISECONDS); | ||
| totalSleptMs += HANDSHAKE_POLL_INTERVAL_MILLISECONDS; | ||
| } | ||
| } | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the SolidSyslogSleepFunction typedef and the config struct's sleep field.
rg -nP -C2 'SolidSyslogSleepFunction|typedef.*sleep|\bsleep\s*[;,]\s*(/\*|$|//)' --type=c --type=cpp -g '!**/Tests/**'
fd -e h -e c 'SolidSyslogTlsStream' --exec rg -nC2 'sleep'Repository: DavidCozens/solid-syslog
Length of output: 3142
Add void* context parameter to SolidSyslogSleepFunction callback per coding guidelines.
The sleep callback must follow the mandatory pattern: "For new callbacks, the function pointer must take a void* context parameter. Config struct must expose a paired context field."
Currently, SolidSyslogSleepFunction is defined as typedef void (*SolidSyslogSleepFunction)(int milliseconds); and called as stream->config.sleep(HANDSHAKE_POLL_INTERVAL_MILLISECONDS); without a context parameter. This prevents passing state into custom sleep implementations (e.g., test fakes, platform objects) and deviates from the library's callback pattern.
Update the typedef, config struct, and call site:
- Add
void* sleepContext;field toSolidSyslogTlsStreamConfig - Change typedef to
typedef void (*SolidSyslogSleepFunction)(int milliseconds, void* context); - Update call to
stream->config.sleep(HANDSHAKE_POLL_INTERVAL_MILLISECONDS, stream->config.sleepContext);
🤖 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 `@Platform/OpenSsl/Source/SolidSyslogTlsStream.c` around lines 340 - 369,
Update the sleep callback to accept a context pointer: change the
SolidSyslogSleepFunction typedef to take (int milliseconds, void* context), add
a void* sleepContext field to SolidSyslogTlsStreamConfig, and update all call
sites (e.g., TlsStream_PerformHandshake) to call
stream->config.sleep(HANDSHAKE_POLL_INTERVAL_MILLISECONDS,
stream->config.sleepContext); ensure any assignments to config.sleep also match
the new signature.
There was a problem hiding this comment.
Declined for the same reason as the typedef thread — sleep is stateless and a void* context parameter would always be NULL.
There was a problem hiding this comment.
@DavidCozens, understood — acknowledged and noted. Since SolidSyslogSleepFunction is intentionally stateless (platform sleep implementations like SolidSyslogPosixSleep and SolidSyslogWindowsSleep carry no per-call state), a void* context parameter would be unnecessary noise. I'll skip this pattern for stateless callbacks going forward.
✏️ Learnings added
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 283
File: Platform/OpenSsl/Source/SolidSyslogTlsStream.c:340-369
Timestamp: 2026-05-07T08:22:56.635Z
Learning: In the `DavidCozens/solid-syslog` repository, `SolidSyslogSleepFunction` (defined in `Core/Interface/SolidSyslogSleep.h`) is intentionally stateless — `typedef void (*SolidSyslogSleepFunction)(int milliseconds);` — and must NOT be flagged for missing a `void* context` parameter. Platform implementations (`SolidSyslogPosixSleep`, `SolidSyslogWindowsSleep`) carry no per-call state, so a context field on `SolidSyslogTlsStreamConfig` would always be NULL. The callback-with-context guideline does not apply to stateless platform utility callbacks in this codebase.
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.
| } | ||
| else | ||
| { | ||
| stream->config.sleep(HANDSHAKE_POLL_INTERVAL_MILLISECONDS); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether any caller (Example, Platform, Bdd) creates a TlsStreamConfig without setting `sleep`.
rg -nP -C5 'SolidSyslogTlsStream_Create\b' -g '!**/Tests/**'
rg -nP -C3 'SolidSyslogTlsStreamConfig\b' -g '!**/Tests/**'Repository: DavidCozens/solid-syslog
Length of output: 17873
🏁 Script executed:
# Examine the context around line 363 in SolidSyslogTlsStream.c
sed -n '350,380p' Platform/OpenSsl/Source/SolidSyslogTlsStream.cRepository: DavidCozens/solid-syslog
Length of output: 992
🏁 Script executed:
# Find all usages of stream->config.sleep
rg -n 'stream->config\.sleep' Platform/OpenSsl/Source/SolidSyslogTlsStream.c -B2 -A2Repository: DavidCozens/solid-syslog
Length of output: 279
🏁 Script executed:
# Check if there are any NULL checks before sleep is called
rg -n 'sleep\s*!=' Platform/OpenSsl/Source/SolidSyslogTlsStream.c -B3 -A3Repository: DavidCozens/solid-syslog
Length of output: 50
🏁 Script executed:
# Get complete Create function to verify no validation exists
sed -n '71,80p' Platform/OpenSsl/Source/SolidSyslogTlsStream.cRepository: DavidCozens/solid-syslog
Length of output: 503
🏁 Script executed:
# Search for all instantiation patterns of SolidSyslogTlsStreamConfig
rg -n 'SolidSyslogTlsStreamConfig\s*=' --type c -A10 | head -80Repository: DavidCozens/solid-syslog
Length of output: 50
🏁 Script executed:
# Check if there are any test files that create TlsStreamConfig
fd -e c -path '*/Test*' -o -path '*/Bdd*' | xargs grep -l 'SolidSyslogTlsStreamConfig' 2>/dev/null || echo "No test files found with TlsStreamConfig"Repository: DavidCozens/solid-syslog
Length of output: 296
🏁 Script executed:
# Examine the header documentation to confirm the "required" designation
sed -n '20,32p' Platform/OpenSsl/Interface/SolidSyslogTlsStream.hRepository: DavidCozens/solid-syslog
Length of output: 994
Add validation in SolidSyslogTlsStream_Create to ensure config->sleep is not NULL.
stream->config.sleep is unconditionally dereferenced during handshake retry (line 363) when WANT_READ/WANT_WRITE occurs. The interface documents sleep as "required" (line 26 of .h), and all current callers (Example code) properly set it. However, _Create performs no validation—if a caller misses this, the service thread crashes. Validate at _Create and return NULL to fail fast.
Suggested fix
struct SolidSyslogStream* SolidSyslogTlsStream_Create(SolidSyslogTlsStreamStorage* storage, const struct SolidSyslogTlsStreamConfig* config)
{
+ if (config->sleep == NULL)
+ {
+ return NULL;
+ }
struct SolidSyslogTlsStream* stream = (struct SolidSyslogTlsStream*) storage;
*stream = DEFAULT_INSTANCE;
stream->config = *config;
return &stream->base;
}🤖 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 `@Platform/OpenSsl/Source/SolidSyslogTlsStream.c` at line 363,
SolidSyslogTlsStream_Create must validate that the provided config and its
required callback sleep are non-NULL to avoid dereferencing stream->config.sleep
during handshake retries; modify SolidSyslogTlsStream_Create to check that
config != NULL and config->sleep != NULL (or stream->config.sleep after copying)
and if missing return NULL (and optionally log an error) so the function fails
fast instead of allowing the service thread to crash when
HANDSHAKE_POLL_INTERVAL_MILLISECONDS is used during WANT_READ/WANT_WRITE
retries.
There was a problem hiding this comment.
Declined — the header documents sleep as required, and the two integrator call sites (Example/Common/ExampleTlsSender_OpenSsl_PosixTcp.c and _WinsockTcp.c) wire it. CLAUDE.md says "don't add validation for scenarios that can't happen — trust internal code and framework guarantees. Only validate at system boundaries (user input, external APIs)." This is a setup-time wiring contract between integrator and library, not an external input.
There was a problem hiding this comment.
@DavidCozens, understood — the project's explicit policy of trusting internal wiring contracts and reserving validation for true system/external boundaries is a sound principle, and the header documentation plus the two concrete call sites make the requirement clear. I'll keep this in mind for future reviews on this repo.
✏️ Learnings added
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 283
File: Platform/OpenSsl/Source/SolidSyslogTlsStream.c:363-363
Timestamp: 2026-05-07T08:22:59.856Z
Learning: In DavidCozens/solid-syslog, CLAUDE.md explicitly states: do not add NULL/defensive validation for scenarios that can't happen under normal internal usage — trust internal code and framework guarantees. Only validate at real system boundaries (user input, external APIs). Setup-time wiring contracts between integrator and library (e.g. required function-pointer fields like `SolidSyslogTlsStreamConfig.sleep`) are intentionally left without NULL guards in `_Create` functions.
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.
- Wrap the four single-statement CHECK_* macros in `do { ... } while (0)`
for consistency with CHECK_SSL_SESSION_CLOSED and the codebase
convention. Guards against the if/else dangling-statement pitfall if
the macro is ever extended.
- Add CHECK_TRANSPORT_CLOSED_ONCE() to the SSL_ERROR_ZERO_RETURN test so
it locks down the full Read fail-fast contract on the clean-shutdown
path, matching the hard-error variant.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1090 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
The previous commit added CHECK_TRANSPORT_CLOSED_ONCE() to the ReadReturnsNegativeOneOnZeroReturnAndClosesSsl test. Touching the TEST_GROUP `transport` member via that macro triggers the same IWYU forward-decl pattern as the other two transport-touching tests in this file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
☀️ Quality Summary 🚦 build-linux-gcc: 100% successful (✔️ 1090 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
DavidCozens
deleted the
feat/s12-14-decouple-buffer-drain-from-sender-state
branch
* chore: bump BDD wait_for_messages deadline 5s → 10s The 5s budget for the buffered example to complete launch + cert load + TLS handshake + Send + oracle write was flake-prone on the Windows OTel runner under load (mTLS scenario in PR #283 timed out once and passed on retry). 10s gives comfortable margin without changing test intent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: make BDD scenarios runnable on Windows-hosted devcontainer Two chronic local-only issues blocked TCP-outage scenarios from running under the Windows-hosted Docker Desktop devcontainer; CI (which runs in its own Linux VM with matching UID/GID) was unaffected. Both fixes are no-ops for CI. 1. shutil.copy → shutil.copyfile in syslog_ng_swap_config (steps) and after_scenario (environment). The destination syslog-ng.conf is bind-mounted from the host and not owned by the container's `developer` user; copy()'s chmod step then fails with EPERM and leaves the config truncated. copyfile copies bytes only, so the file's existing mode (set by the host) is left intact. 2. syslog-ng compose entrypoint now keeps a watchdog re-chmodding /var/lib/syslog-ng/syslog-ng.ctl to 0777. syslog-ng RELOAD recreates the socket with default 0755 perms; the behave container connects as `developer` and needs world-writable to call connect(2). Single 100ms-tick background loop, exits with the syslog-ng pid. Closes the open follow-up tracked in MEMORY:project_syslog_ng_conf_corruption. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat: S13.21 re-enable @windows_wip store_capacity scenarios S12.14's eager-drain refactor decoupled buffer drain from sender state on both runners; the four store_capacity discard scenarios that were tagged @windows_wip pending that fix now run reliably end-to-end. Drop the four @windows_wip tags and the long explanatory note; the existing 10/8-message form gives sufficient margin for the discard policy to engage with both runners. Locally verified all 4 scenarios pass on Linux (and the full BDD suite — 21 features, 46 scenarios, 242 steps — remains green). Closes #278 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: address CodeRabbit review on PR #284 - environment.py after_scenario: move shutil.copyfile inside the existing try/except so a copyfile failure is logged via the warning path rather than bypassing the rest of teardown. - syslog_steps.py step_oracle_stops_tcp: set context.syslog_ng_config_changed = True *before* syslog_ng_swap_config() rather than after. Any failure inside swap (or the subsequent wait / probe teardown / sleep) still triggers the after_scenario restore path — otherwise a half-applied swap would leave the on-disk config in UDP-only mode and contaminate the next scenario. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
ProcessMessagesnow drains Buffer→Store in a loop, then attempts one Send from the Store. Bursts no longer drop at the in-memory buffer; the Store's discard policy can engage.Sendreturnstrueonly when the kernel accepts the entire frame in one call; any failure (short write, would-block, error) closes the socket internally and returnsfalse.Readreturns0on would-block,-1with internal close on EOF/error. TLS handshake is driven in a bounded retry loop (5 s budget, 1 ms poll).Core/Interface/SolidSyslogStream.h. Caller responsibility on afalseSend /-1Read: callOpenbefore the next Send.Closes #276
Why this matters
ProcessMessagespreviously did one buffer→store write + one send per service tick. When a TCP/TLS sender stalled (slow SIEM, refused loopback retry, full kernel send buffer), the buffer→store pump stalled too. Producers'SolidSyslog_Logcalls then filled the in-memory buffer, which silently drops on full — the BlockStore's discard policy never saw the records, so an outage couldn't engagediscard-oldest/discard-newest/halt.Discovered via the four
@windows_wipstore_capacity.featurescenarios on Windows in PR #275.Slice-by-slice
ProcessMessagesdrain + StoreFake FIFO + new burst-drain testsfcntl O_NONBLOCK, boundedselect()connect,getsockopt(SO_ERROR), fail-fast Send/Read; SocketFake gainsfcntl/select/SO_ERRORseamsSO_SNDTIMEO; WinsockFake gainsFailNextRecvWithLastErrorBIO_set_retry_read, boundedSSL_connectretry loop, fail-fastSSL_write/read, idempotentClose; OpenSslFake gains return-sequence injection,SSL_get_error, BIO retry-flag spiesCoverage 100% (2024/2024 lines, 429/429 functions). Format / IWYU / sanitize / tidy / cppcheck / clang-debug all green.
Disagreements held during design review
Deferred (follow-up stories being filed in parallel)
SO_KEEPALIVE+TCP_KEEPIDLE/INTVL/CNT+SIO_KEEPALIVE_VALS+TCP_USER_TIMEOUT) for dead-peer idle detection.Out of scope intentionally
The four
@windows_wiptags inBdd/features/store_capacity.featureare not removed in this PR. Per #276, the tag removal and BDD re-tuning are the finale of #275. This PR delivers the architectural fix that unblocks them.Test plan
store_capacity.featurereaches "the syslog oracle receives 1 message" against the Windows MSVC build + otelcol-contrib (architectural decoupling confirmed)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation