Add readiness warmup to restricted-config test to fix CI flakiness#154
Merged
Conversation
WalkthroughThe integration test increases event-publishing retry tolerance and adds a mode-specific warmup gate that must succeed before existing assertions run. ChangesRelay readiness checks
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🤖 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 `@tests/integration_restrict.sh`:
- Around line 42-67: Reduce the per-attempt timeout in the warmup function so
its 40 retries, including sleeps, complete within the documented approximately
20-second limit. Keep the existing success detection, retry count, and failure
behavior unchanged, and update the nearby duration comment if needed to
accurately reflect the enforced deadline.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e087357-bd50-4c5c-b0db-5caacc784291
📒 Files selected for processing (1)
tests/integration_restrict.sh
Comment on lines
+42
to
+67
| # Block until the relay actually accepts an event of this mode's accepted class, | ||
| # not just until the TCP port is open. In CI the relay is started fresh per mode | ||
| # and `nc -z` only proves the listener is up, not that the event pipeline is | ||
| # ready; grading the first assertion against a still-warming relay is what made | ||
| # this suite flaky on main (a transient miss on an expected-accept assertion | ||
| # reports 'reject' and fails CI). Publishing an acceptable throwaway event until | ||
| # it succeeds converts "port open" into "pipeline ready", and is independent of | ||
| # noz's error-output format so it needs no reject-vs-timeout parsing. | ||
| warmup() { | ||
| local args | ||
| case "$MODE" in | ||
| auth) args=(--sec "$SEC1" --auth -c warmup) ;; | ||
| protected) args=(--sec "$SEC1" -c warmup) ;; | ||
| pow) args=(--sec "$SEC1" --pow 8 -c warmup) ;; | ||
| *) return 0 ;; # unknown mode is reported by the assertion case below | ||
| esac | ||
| for i in $(seq 1 40); do | ||
| timeout 12 noz event "${args[@]}" "$R" 2>&1 | grep -q 'success' && return 0 | ||
| sleep 0.5 | ||
| done | ||
| echo "warmup: relay never accepted an event in '$MODE' mode after ~20s" >&2 | ||
| return 1 | ||
| } | ||
|
|
||
| warmup || exit 1 | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Worst-case warmup duration far exceeds the ~20s comment and "fail fast" goal.
Each of the 40 iterations can take up to 12s (timeout 12) plus 0.5s sleep, yielding a ~500s worst case if the relay accepts connections but never responds. The comment on line 62 says "~20s" but that only accounts for sleep time. If the relay is in a bad state, CI will hang for 8+ minutes before failing — contradicting the PR's "fail fast" objective.
Consider using a shorter per-attempt timeout for the warmup (readiness probe, not a full operation) or tracking total elapsed time with a hard deadline.
⏱️ Suggested fix: shorter warmup timeout
for i in $(seq 1 40); do
- timeout 12 noz event "${args[@]}" "$R" 2>&1 | grep -q 'success' && return 0
+ timeout 3 noz event "${args[@]}" "$R" 2>&1 | grep -q 'success' && return 0
sleep 0.5
done📝 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.
Suggested change
| # Block until the relay actually accepts an event of this mode's accepted class, | |
| # not just until the TCP port is open. In CI the relay is started fresh per mode | |
| # and `nc -z` only proves the listener is up, not that the event pipeline is | |
| # ready; grading the first assertion against a still-warming relay is what made | |
| # this suite flaky on main (a transient miss on an expected-accept assertion | |
| # reports 'reject' and fails CI). Publishing an acceptable throwaway event until | |
| # it succeeds converts "port open" into "pipeline ready", and is independent of | |
| # noz's error-output format so it needs no reject-vs-timeout parsing. | |
| warmup() { | |
| local args | |
| case "$MODE" in | |
| auth) args=(--sec "$SEC1" --auth -c warmup) ;; | |
| protected) args=(--sec "$SEC1" -c warmup) ;; | |
| pow) args=(--sec "$SEC1" --pow 8 -c warmup) ;; | |
| *) return 0 ;; # unknown mode is reported by the assertion case below | |
| esac | |
| for i in $(seq 1 40); do | |
| timeout 12 noz event "${args[@]}" "$R" 2>&1 | grep -q 'success' && return 0 | |
| sleep 0.5 | |
| done | |
| echo "warmup: relay never accepted an event in '$MODE' mode after ~20s" >&2 | |
| return 1 | |
| } | |
| warmup || exit 1 | |
| # Block until the relay actually accepts an event of this mode's accepted class, | |
| # not just until the TCP port is open. In CI the relay is started fresh per mode | |
| # and `nc -z` only proves the listener is up, not that the event pipeline is | |
| # ready; grading the first assertion against a still-warming relay is what made | |
| # this suite flaky on main (a transient miss on an expected-accept assertion | |
| # reports 'reject' and fails CI). Publishing an acceptable throwaway event until | |
| # it succeeds converts "port open" into "pipeline ready", and is independent of | |
| # noz's error-output format so it needs no reject-vs-timeout parsing. | |
| warmup() { | |
| local args | |
| case "$MODE" in | |
| auth) args=(--sec "$SEC1" --auth -c warmup) ;; | |
| protected) args=(--sec "$SEC1" -c warmup) ;; | |
| pow) args=(--sec "$SEC1" --pow 8 -c warmup) ;; | |
| *) return 0 ;; # unknown mode is reported by the assertion case below | |
| esac | |
| for i in $(seq 1 40); do | |
| timeout 3 noz event "${args[@]}" "$R" 2>&1 | grep -q 'success' && return 0 | |
| sleep 0.5 | |
| done | |
| echo "warmup: relay never accepted an event in '$MODE' mode after ~20s" >&2 | |
| return 1 | |
| } | |
| warmup || exit 1 |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 58-58: i appears unused. Verify use (or export if used externally).
(SC2034)
🤖 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/integration_restrict.sh` around lines 42 - 67, Reduce the per-attempt
timeout in the warmup function so its 40 retries, including sleeps, complete
within the documented approximately 20-second limit. Keep the existing success
detection, retry count, and failure behavior unchanged, and update the nearby
duration comment if needed to accurately reflect the enforced deadline.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The
restricted-configintegration step (NIP-42 / 70 / 13) is flaky onmain. The push run for the previous merge failed at "Run restricted-config tests" while the identical commit had just passed the same job on its PR branch (29199249925failed on main vs29199117246green on the PR).Root cause:
run_modeinci.ymlstarts a fresh relay per mode and grades assertions as soon asnc -zreports the TCP port open. But TCP-listening is not the same as the event pipeline being ready. When the first graded assertion races relay warmup (or hits a transient connect/read timeout under runner load), an expected-accept assertion reportsrejectand fails CI. Expected-reject assertions are immune (a timeout already yieldsreject), so only the accept cases flake.Fix
Test-only change to
tests/integration_restrict.sh:--auth, protected → normal, pow → mined) and retry until the relay accepts it. This converts "port open" into "pipeline ready" and is independent ofnoz's error-output format, so it needs no reject-vs-timeout parsing. If the relay never becomes ready it fails fast (~20s) with a clear message instead of a confusing assertion failure.pubres. 3 → 4 attempts,timeout10 → 12s, to absorb the rarer mid-run blip. Kept conservative so expected-reject assertions stay fast (the warmup does the heavy lifting).Verification
bash -nclean.nozacross all three modes: correct verdicts, exit 0.No production code changed; no relay behavior changed.
Summary by CodeRabbit