fix: apply notify task after lock is released #265

joseph-sentry · 2024-02-12T16:35:47Z

there is the possibility of an issue when the
notify task is queued up before the test result finisher releases the notification lock due to the fact that a notify will drop itself if it fails to lock, so we should make sure that the test result finisher is not holding the notification lock before it queues up the notify task

Sentry Issue

Fixes: https://github.com/codecov/internal-issues/issues/364

codecov · 2024-02-12T16:46:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.65%. Comparing base (250b8e1) to head (3d4a3bc).

✅ All tests successful. No failed tests found ☺️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #265   +/-   ##
=======================================
  Coverage   97.65%   97.65%           
=======================================
  Files         424      424           
  Lines       33532    33536    +4     
=======================================
+ Hits        32746    32750    +4     
  Misses        786      786

Flag	Coverage Δ
integration	`97.64% <100.00%> (+<0.01%)`	⬆️
latest-uploader-overall	`97.64% <100.00%> (+<0.01%)`	⬆️
unit	`97.64% <100.00%> (+<0.01%)`	⬆️

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

Components	Coverage Δ
NonTestCode	`95.12% <100.00%> (+<0.01%)`	⬆️
OutsideTasks	`97.79% <ø> (ø)`

Files	Coverage Δ
tasks/test_results_finisher.py	`96.62% <100.00%> (+0.15%)`	⬆️
tasks/tests/unit/test_test_results_finisher.py	`100.00% <100.00%> (ø)`

Related Entrypoints
run/app.tasks.test_results.TestResultsFinisherTask

codecov-qa · 2024-02-12T16:50:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.64%. Comparing base (250b8e1) to head (3d4a3bc).

✅ All tests successful. No failed tests found ☺️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #265   +/-   ##
=======================================
  Coverage   97.64%   97.64%           
=======================================
  Files         393      393           
  Lines       32832    32836    +4     
=======================================
+ Hits        32058    32062    +4     
  Misses        774      774

Flag	Coverage Δ
integration	`97.64% <100.00%> (+<0.01%)`	⬆️
latest-uploader-overall	`97.64% <100.00%> (+<0.01%)`	⬆️
unit	`97.64% <100.00%> (+<0.01%)`	⬆️

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

Components	Coverage Δ
NonTestCode	`95.13% <100.00%> (+<0.01%)`	⬆️
OutsideTasks	`97.79% <ø> (ø)`

Files	Coverage Δ
tasks/test_results_finisher.py	`95.50% <100.00%> (+0.21%)`	⬆️
tasks/tests/unit/test_test_results_finisher.py	`100.00% <100.00%> (ø)`

codecov-public-qa · 2024-02-12T16:50:06Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.64%. Comparing base (250b8e1) to head (3d4a3bc).

✅ All tests successful. No failed tests found ☺️

@@           Coverage Diff           @@
##             main     #265   +/-   ##
=======================================
  Coverage   97.64%   97.64%           
=======================================
  Files         393      393           
  Lines       32832    32836    +4     
=======================================
+ Hits        32058    32062    +4     
  Misses        774      774

Flag	Coverage Δ
integration	`97.64% <100.00%> (+<0.01%)`	⬆️
latest-uploader-overall	`97.64% <100.00%> (+<0.01%)`	⬆️
unit	`97.64% <100.00%> (+<0.01%)`	⬆️

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

Components	Coverage Δ
NonTestCode	`95.13% <100.00%> (+<0.01%)`	⬆️
OutsideTasks	`97.79% <ø> (ø)`

Files	Coverage Δ
tasks/test_results_finisher.py	`95.50% <100.00%> (+0.21%)`	⬆️
tasks/tests/unit/test_test_results_finisher.py	`100.00% <100.00%> (ø)`

codecov-staging · 2024-02-22T21:23:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found ☺️

@@           Coverage Diff           @@
##             main     #265   +/-   ##
=======================================
  Coverage   97.64%   97.64%           
=======================================
  Files         393      393           
  Lines       32832    32836    +4     
=======================================
+ Hits        32058    32062    +4     
  Misses        774      774

Flag	Coverage Δ
integration	`97.64% <100.00%> (+<0.01%)`	⬆️
latest-uploader-overall	`97.64% <100.00%> (+<0.01%)`	⬆️
unit	`97.64% <100.00%> (+<0.01%)`	⬆️

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

Components	Coverage Δ
NonTestCode	`95.13% <100.00%> (+<0.01%)`	⬆️
OutsideTasks	`97.79% <ø> (ø)`

Files	Coverage Δ
tasks/test_results_finisher.py	`95.50% <100.00%> (+0.21%)`	⬆️
tasks/tests/unit/test_test_results_finisher.py	`100.00% <100.00%> (ø)`

giovanni-guidini · 2024-04-05T11:02:50Z

There's 1 more change I'd like to see in this PR, which is changing

99: except LockError as err:

to be

99: except LockRetry as err:

You see, that is when the notification task realizes that there's possibly another notification task happening. However when line 87 was changed to use lock_manager the exception handling broke. Previously it used redis directly (which raised LockError), but the lock_manager raises the LockRetry error see here.

So we should handle the exception better.
Otherwise this looks good.

giovanni-guidini

Approving on the basis that the other change I commented on has nothing to do with these changes so should probably be on another PR.
This seems fine. Apologies for suggesting unrelated changes 😅

there is the possibility of an issue when the notify task is queued up before the test result finisher releases the notification lock due to the fact that a notify will drop itself if it fails to lock, so we should make sure that the test result finisher is not holding the notification lock before it queues up the notify task Signed-off-by: joseph-sentry <[email protected]>

Signed-off-by: joseph-sentry <[email protected]>

joseph-sentry requested a review from matt-codecov February 15, 2024 16:48

joseph-sentry force-pushed the joseph/fix-test-results-notify branch from 0eea563 to 17d9a0e Compare April 2, 2024 20:19

joseph-sentry requested a review from a team April 3, 2024 13:16

joseph-sentry force-pushed the joseph/fix-test-results-notify branch from c63cdb3 to fc7af92 Compare April 8, 2024 15:27

giovanni-guidini approved these changes Apr 8, 2024

View reviewed changes

joseph-sentry force-pushed the joseph/fix-test-results-notify branch 2 times, most recently from 0469f50 to 27899c1 Compare April 9, 2024 13:51

joseph-sentry added 2 commits April 9, 2024 10:14

test: fix test result finisher tests

3d4a3bc

Signed-off-by: joseph-sentry <[email protected]>

joseph-sentry force-pushed the joseph/fix-test-results-notify branch from 27899c1 to 3d4a3bc Compare April 9, 2024 14:14

joseph-sentry merged commit 60991b5 into main Apr 9, 2024
30 checks passed

joseph-sentry deleted the joseph/fix-test-results-notify branch April 9, 2024 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: apply notify task after lock is released #265

fix: apply notify task after lock is released #265

joseph-sentry commented Feb 12, 2024 •

edited

Loading

codecov bot commented Feb 12, 2024 •

edited

Loading

codecov-qa bot commented Feb 12, 2024 •

edited

Loading

codecov-public-qa bot commented Feb 12, 2024 •

edited

Loading

codecov-staging bot commented Feb 22, 2024 •

edited by codecov-notifications bot

Loading

giovanni-guidini commented Apr 5, 2024 •

edited

Loading

giovanni-guidini left a comment

fix: apply notify task after lock is released #265

fix: apply notify task after lock is released #265

Conversation

joseph-sentry commented Feb 12, 2024 • edited Loading

codecov bot commented Feb 12, 2024 • edited Loading

Codecov Report

codecov-qa bot commented Feb 12, 2024 • edited Loading

Codecov Report

codecov-public-qa bot commented Feb 12, 2024 • edited Loading

Codecov Report

codecov-staging bot commented Feb 22, 2024 • edited by codecov-notifications bot Loading

Codecov Report

giovanni-guidini commented Apr 5, 2024 • edited Loading

giovanni-guidini left a comment

Choose a reason for hiding this comment

joseph-sentry commented Feb 12, 2024 •

edited

Loading

codecov bot commented Feb 12, 2024 •

edited

Loading

codecov-qa bot commented Feb 12, 2024 •

edited

Loading

codecov-public-qa bot commented Feb 12, 2024 •

edited

Loading

codecov-staging bot commented Feb 22, 2024 •

edited by codecov-notifications bot

Loading

giovanni-guidini commented Apr 5, 2024 •

edited

Loading