[high] fix: wait for the publish worker in test_search_publish_timestamp - #1482
Open
elhoim wants to merge 1 commit into
Open
[high] fix: wait for the publish worker in test_search_publish_timestamp#1482elhoim wants to merge 1 commit into
elhoim wants to merge 1 commit into
Conversation
The test asserts that a search for publish_timestamp='5s' returns exactly
one event immediately after creating it. Since 2.5.40 MISP sets the publish
timestamp in a background worker rather than during add_event, so this only
passed because the three invalid-query searches in between happened to take
longer than the worker's tick.
That makes the test sensitive to unrelated server-side performance. It fails
reproducibly against a MISP branch that removes a bcrypt verification from
every authenticated REST request: the same 85-test suite drops from 190-217s
to 146s, the three queries return before the worker has published, and the
search correctly matches nothing - "AssertionError: 0 != 1".
Wait for publication instead of relying on incidental latency:
- add _wait_until_published(), which polls until the event reports
published with a non-zero publish_timestamp, and fails with a clear
message on timeout rather than letting a later assertion fail obscurely.
It accepts publish_timestamp as either the int 0 add_event returns or the
datetime pythonify produces once the worker has run.
- wait for the first event before creating the second, so the two publish
timestamps are genuinely separated. The previous sleep separated only the
creation times, which is not what the interval assertion compares.
- wait for the second event before the 5-second window assertion.
- drop the fixed 10s sleep that preceded the re-fetch. It slowed the suite
down and still raised AttributeError on int.timestamp() whenever the
worker was slower than the guess.
- assert the two publish timestamps are more than 5s apart, so a violation
reports the real gap instead of an opaque count mismatch.
The test is now driven by the worker's actual progress, so it neither races a
faster server nor pays a fixed delay on a slower one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
BLUF —
test_search_publish_timestampraces the publish worker and fails whenever the server answers faster.publish_timestamp='5s'search returns exactly one event immediately after creating it, but since MISP 2.5.40 the publish timestamp is set by a background worker rather than duringadd_event. The assertion only ever passed because the three invalid-query searches before it happened to take longer than the worker's tick.How this surfaced
MISP/MISP#11081 removes a bcrypt verification from every authenticated REST request. On that branch this test fails reproducibly on both CI legs:
The runtime of this very suite is the evidence that nothing is actually broken:
testlive_comprehensive(same 85 tests)The faster the server answers, the sooner the three invalid queries return, and the less time the publish worker has had.
0is the correct answer to the search at that moment — the event genuinely is not published yet.Why it is a race
Nothing between
add_eventand the assertion waits for publication. The three invalid queries are load-bearing purely as a delay, which is not what they were written for. The test's own comment a few lines further down already acknowledges the 2.5.40 change and sleeps for it — but only after the assertion that needs it.The change
_wait_until_published()— polls until the event reportspublishedwith a non-zeropublish_timestamp, then returns the re-fetched event. On timeout it fails withevent <id> was not published within <n>srather than letting a later assertion fail obscurely. It acceptspublish_timestampas either theint0thatadd_eventreturns or thedatetimethatpythonifyproduces once the worker has run.sleeponly separated their creation times.time.sleep(10)that preceded the re-fetch. It slowed the suite down and still raisedAttributeErroronint.timestamp()whenever the worker was slower than the guess.Verification
The helper's control flow was exercised against a stubbed connector, since the surrounding suite needs a live MISP instance:
published=Truebutpublish_timestamp == 0event 42 was not published within 2sThe test is now driven by the worker's actual progress, so it neither races a faster server nor pays a fixed delay on a slower one.
MISP/MISP#11081 is blocked on this change and should go green once it is merged and MISP's PyMISP submodule picks it up.
🤖 Generated with Claude Code