Skip to content
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.

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

Fix the parser so that it doesn't loop forever (bugfix) #1555

Merged
merged 2 commits into from
Oct 22, 2024

Conversation

Hook25
Copy link
Collaborator

@Hook25 Hook25 commented Oct 21, 2024

Description

When validating units, Checkbox tries to parse fields to see if there are any errors with them. Turns out that if a field contains an un-terminated string"..., that will cause the parser to always loop forever or crash with an assertion error. This was affecting a test run today due to a non-sluggified name getting templated into a job. This manifested itself as an infinited crash-restore loop due to also an un-related error (if the session gets stuck on an infinite loop, the controller tries to reconnect eventually, Checkbox refuses to "recover" during bootstrapping, restarting the session)

Resolved issues

Fixes: https://warthogs.atlassian.net/browse/CHECKBOX-1622

Documentation

This also documents how the parser works because it is quite something

Tests

This also adds the tests I used to reproduce the issue

@Hook25 Hook25 force-pushed the fix_nonterminated_strings_parsing branch from 77b92e7 to 7820284 Compare October 21, 2024 15:55
Copy link

codecov bot commented Oct 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.78%. Comparing base (b6d5bfa) to head (a8a7abe).
Report is 90 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1555      +/-   ##
==========================================
+ Coverage   47.76%   47.78%   +0.02%     
==========================================
  Files         370      370              
  Lines       39750    39751       +1     
  Branches     6720     6720              
==========================================
+ Hits        18987    18997      +10     
+ Misses      20048    20042       -6     
+ Partials      715      712       -3     
Flag Coverage Δ
checkbox-ng 68.28% <100.00%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Minor: adding myself to written by as this was so hard to track down
@Hook25 Hook25 force-pushed the fix_nonterminated_strings_parsing branch from 64423b6 to a8a7abe Compare October 21, 2024 16:13
Copy link
Collaborator

@fernando79513 fernando79513 left a comment

Choose a reason for hiding this comment

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

Great investigation, what a piece of code!
LGTM+1!

@Hook25 Hook25 merged commit 60b6702 into main Oct 22, 2024
48 checks passed
@Hook25 Hook25 deleted the fix_nonterminated_strings_parsing branch October 22, 2024 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants