Sign task-erred with run_id and reject outdated responses
#7933
Conversation
hendrikmakait
changed the title
task-erred with run_id and reject outdated responses #task-erred with run_id and reject outdated responses
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 20 files + 20 20 suites +20 11h 27m 10s ⏱️ + 11h 27m 10s For more details on these failures and errors, see this check. Results for commit 23259b1. ± Comparison against base commit cf97a7c. ♻️ This comment has been updated with latest results. |
| instructions.append(TaskErredMsg.from_task(ts, stimulus_id=ev.stimulus_id)) | ||
| instructions.append( | ||
| TaskErredMsg.from_task(ts, run_id=ev.run_id, stimulus_id=ev.stimulus_id) | ||
| ) |
There was a problem hiding this comment.
coverage says we never test this use case.
This is not something you did; but would you mind taking the opportunity to add a unit test for it? It's a fairly big blind spot.
There was a problem hiding this comment.
I'm not even sure if the code here is correct. I would assume that in this case (especially with the task-erred signing) this task should rather be released and recomputed instead of being reported erred.
There was a problem hiding this comment.
Agreed, I think that in this case as well as the case above for ts.state == "memory", we should probably recompute instead of acting based on the existing result. I will create a follow-up issue.
There was a problem hiding this comment.
Changes LGTM
I agree with @crusaderky that we should write a test for that one case but I suggest to do this in a separate PR. As it is, this PR is improving main and the additional edge case is an increase in scope, particularly since I'm not entirely certain if the current code is even correct.
Considering this is not urgent, I'll leave merging up to @hendrikmakait once he's back
| instructions.append(TaskErredMsg.from_task(ts, stimulus_id=ev.stimulus_id)) | ||
| instructions.append( | ||
| TaskErredMsg.from_task(ts, run_id=ev.run_id, stimulus_id=ev.stimulus_id) | ||
| ) |
There was a problem hiding this comment.
I'm not even sure if the code here is correct. I would assume that in this case (especially with the task-erred signing) this task should rather be released and recomputed instead of being reported erred.
Co-authored-by: crusaderky <crusaderky@gmail.com>
Closes #7489
pre-commit run --all-files