Skip to content

fix: Ensure persist state event emission when exiting EventManager context #1562

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
Pijukatel merged 6 commits into from
Nov 18, 2025
Merged

fix: Ensure persist state event emission when exiting EventManager context #1562

Pijukatel merged 6 commits into from
Nov 18, 2025

Conversation

@Pijukatel
Copy link
Collaborator

@Pijukatel Pijukatel commented Nov 14, 2025 •
edited by vdusek
Loading

Description

  • Fixes root cause for flakiness in test_crawler_statistics_persistence
  • Previously, BasicCrawler was manually emitting a persist state event when exiting. Now this should be done by the EventManager.
  • Statistics used to be double-persisted previously, and due to a race condition, the old state could be persisted sometimes. Now the statistics are persisted by themselves when exiting their own context, and not by the Crawler-emitted event
  • StatisticsState.crawler_runtime changed to a computed field in a backwards compatible way. This allows pushing the runtime calculation to the state and ensures consistency between attribute access and persistence. (This prevents a theoretical race condition when automatic persistence triggers almost at the same time as the crawler finishes, and again causes the scenario described above).

Issues

Testing

  • Stress testing in CI
  • Added unit test

Checklist

  • CI passed

@Pijukatel
Persist automatically when exiting EventManager context
cf3e119 
Add test.
Remove redundant persist event in BasicCrawler.
Add stress test to flaky test
@Pijukatel
Change crawler_runtime to computed_field as it really is one
f7acfc5
@Pijukatel Pijukatel closed this Nov 14, 2025
@github-actions github-actions bot added this to the 127th sprint - Tooling team milestone Nov 14, 2025
@github-actions github-actions bot added t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics. labels Nov 14, 2025
@Pijukatel Pijukatel changed the title Fix flaky statistics test fix: Ensure persist state event emission when exiting EventManager context Nov 14, 2025
@Pijukatel Pijukatel reopened this Nov 14, 2025
@Pijukatel Pijukatel marked this pull request as ready for review November 14, 2025 14:26
@Pijukatel Pijukatel requested a review from vdusek November 14, 2025 14:30
vdusek
vdusek approved these changes Nov 14, 2025
View reviewed changes
Copy link
Collaborator

@vdusek vdusek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍, LGTM

Mantisus
Mantisus approved these changes Nov 14, 2025
View reviewed changes
Copy link
Collaborator

@Mantisus Mantisus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Pijukatel Pijukatel mentioned this pull request Nov 18, 2025
Open
@Pijukatel Pijukatel merged commit 6a44f17 into master Nov 18, 2025
23 checks passed
@Pijukatel Pijukatel deleted the fix-flaky-statistics-test branch November 18, 2025 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vdusek vdusek vdusek approved these changes

+1 more reviewer

@Mantisus Mantisus Mantisus approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Pijukatel Pijukatel

Labels

t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.

Projects

None yet

Milestone

127th sprint - Tooling team

Development

Successfully merging this pull request may close these issues.

Test test_crawler_statistics_persistence is flaky

4 participants

@Pijukatel @vdusek @Mantisus