feat: S13.21 re-enable @windows_wip store_capacity scenarios#284
Conversation
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>
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>
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>
|
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 (2)
📝 WalkthroughWalkthroughThis PR fixes BDD infrastructure robustness by addressing socket permission handling in Docker Compose (switching to a continuous watchdog loop), updates file copy operations to avoid bind-mount permission errors, extends message receive timeout, and re-enables four store capacity test scenarios by removing ChangesBDD Infrastructure Robustness & Store Capacity Re-enablement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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. Comment |
![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/environment.py`:
- Around line 136-140: The shutil.copyfile call that restores SYSLOG_NG_CONF
from SYSLOG_NG_FULL_CONF is executed outside the guarded teardown block and can
raise exceptions that bypass the subsequent try/except cleanup; move the
shutil.copyfile(...) into the same try block that handles reload/wait so that
any copy failure is caught and handled alongside the reload logic when
context.syslog_ng_config_changed is true, keeping the condition check on
context.syslog_ng_config_changed and leaving the reload/wait and exception
handling intact.
In `@Bdd/features/steps/syslog_steps.py`:
- Around line 470-480: The syslog_ng_swap_config function can leave the system
in a bad state if shutil.copyfile succeeds but syslog_ng_reload() fails; make
the swap transactional by first creating a safe backup of SYSLOG_NG_CONF (e.g.,
temp file), then copy the new config, attempt syslog_ng_reload() inside a try
block, and if reload raises/returns failure restore the backup (and optionally
retry reload) so the original config is reinstated; ensure the backup is removed
on success and that any exceptions still allow teardown to see the context flag
by re-raising after cleanup. Use the existing function names
syslog_ng_swap_config, syslog_ng_reload and the SYSLOG_NG_CONF constant to
locate where to add the try/except/finally backup-and-restore logic.
🪄 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: 81ecf186-e964-4c59-8bf2-6248484ef8e7
📒 Files selected for processing (4)
.devcontainer/docker-compose.ymlBdd/features/environment.pyBdd/features/steps/syslog_steps.pyBdd/features/store_capacity.feature
💤 Files with no reviewable changes (1)
- Bdd/features/store_capacity.feature
☀️ 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. |
- 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>
☀️ 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. |
1 participant
Summary
@windows_wiptags + the long explanatory note fromBdd/features/store_capacity.feature. S12.14's eager-drain refactor is onmain, so the architectural blocker is gone; the existing 10/8-message form works on both runners with no retune needed.shutil.copy→shutil.copyfileinsyslog_ng_swap_configand theafter_scenariohook. The bind-mountedsyslog-ng.confis owned by the host user, socopy()'s chmod step fails withEPERMand truncates the config;copyfilecopies bytes only and leaves the host-set mode intact.chmodingsyslog-ng.ctlto0777for the lifetime of the container.RELOADrecreates the socket with default0755, which thedeveloper-uid behave container can'tconnect(2)to.wait_for_messagesdeadline5s → 10s. Pre-emptive flake mitigation; the buffered example's launch + cert load + TLS handshake + Send + oracle write was tight on slow Windows runners (mTLS scenario in PR feat: S12.14 decouple buffer drain from sender state — fail-fast streams + eager Service drain #283 timed out once and passed on retry). 10s gives comfortable margin without changing test intent.Test plan
store_capacityscenarios pass locally on Linux (gcc devcontainer + behave compose service).Closes #278
Summary by CodeRabbit
Chores
Tests