Repository navigation
fix: worker cancellation sets status to 'cancelled' instead of 'failed' #399
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
Changes from 2 commits
7063c3a
5db46ca
d4ff62e
5447bbb
b0530d1
4817b9e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -475,10 +475,20 @@ impl ProcessRunLogger { | |||||
|
|
||||||
| /// Record a worker completing with its result. Fire-and-forget. | ||||||
| pub fn log_worker_completed(&self, worker_id: WorkerId, result: &str, success: bool) { | ||||||
| let status = if success { "done" } else { "failed" }; | ||||||
| self.log_worker_completed_with_status(worker_id, result, status); | ||||||
| } | ||||||
|
|
||||||
| /// Record a worker as cancelled. Fire-and-forget. | ||||||
| pub fn log_worker_cancelled(&self, worker_id: WorkerId, result: &str) { | ||||||
| self.log_worker_completed_with_status(worker_id, result, "cancelled"); | ||||||
| } | ||||||
|
coderabbitai[bot] marked this conversation as resolved.
Resolved
|
||||||
|
|
||||||
| fn log_worker_completed_with_status(&self, worker_id: WorkerId, result: &str, status: &str) { | ||||||
| let pool = self.pool.clone(); | ||||||
| let id = worker_id.to_string(); | ||||||
| let result = result.to_string(); | ||||||
| let status = if success { "done" } else { "failed" }; | ||||||
| let status = status.to_string(); | ||||||
|
|
||||||
| tokio::spawn(async move { | ||||||
| if let Err(error) = sqlx::query( | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since Might be worth adding a small retry on
Suggested change
|
||||||
|
|
@@ -1054,6 +1064,82 @@ mod tests { | |||||
| assert_eq!(result, "Worker cancelled"); | ||||||
| } | ||||||
|
|
||||||
| #[tokio::test] | ||||||
| async fn log_worker_completed_success_sets_done_status() { | ||||||
| let pool = setup_worker_runs_table().await; | ||||||
| let logger = ProcessRunLogger::new(pool.clone()); | ||||||
| let worker_id = uuid::Uuid::new_v4(); | ||||||
|
|
||||||
| sqlx::query("INSERT INTO worker_runs (id, channel_id, status, result) VALUES (?, 'ch-1', 'running', '')") | ||||||
| .bind(worker_id.to_string()) | ||||||
| .execute(&pool) | ||||||
| .await | ||||||
| .expect("insert"); | ||||||
|
|
||||||
| logger.log_worker_completed(worker_id, "task finished", true); | ||||||
| tokio::time::sleep(std::time::Duration::from_millis(50)).await; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This |
||||||
|
|
||||||
| let row = sqlx::query("SELECT status, result FROM worker_runs WHERE id = ?") | ||||||
| .bind(worker_id.to_string()) | ||||||
| .fetch_one(&pool) | ||||||
| .await | ||||||
| .expect("fetch"); | ||||||
| let status: String = sqlx::Row::try_get(&row, "status").expect("status"); | ||||||
| let result: String = sqlx::Row::try_get(&row, "result").expect("result"); | ||||||
| assert_eq!(status, "done"); | ||||||
| assert_eq!(result, "task finished"); | ||||||
| } | ||||||
|
|
||||||
| #[tokio::test] | ||||||
| async fn log_worker_completed_failure_sets_failed_status() { | ||||||
| let pool = setup_worker_runs_table().await; | ||||||
| let logger = ProcessRunLogger::new(pool.clone()); | ||||||
| let worker_id = uuid::Uuid::new_v4(); | ||||||
|
|
||||||
| sqlx::query("INSERT INTO worker_runs (id, channel_id, status, result) VALUES (?, 'ch-1', 'running', '')") | ||||||
| .bind(worker_id.to_string()) | ||||||
| .execute(&pool) | ||||||
| .await | ||||||
| .expect("insert"); | ||||||
|
|
||||||
| logger.log_worker_completed(worker_id, "something broke", false); | ||||||
| tokio::time::sleep(std::time::Duration::from_millis(50)).await; | ||||||
|
|
||||||
| let row = sqlx::query("SELECT status FROM worker_runs WHERE id = ?") | ||||||
| .bind(worker_id.to_string()) | ||||||
| .fetch_one(&pool) | ||||||
| .await | ||||||
| .expect("fetch"); | ||||||
| let status: String = sqlx::Row::try_get(&row, "status").expect("status"); | ||||||
| assert_eq!(status, "failed"); | ||||||
| } | ||||||
|
|
||||||
| #[tokio::test] | ||||||
| async fn log_worker_cancelled_sets_cancelled_status() { | ||||||
| let pool = setup_worker_runs_table().await; | ||||||
| let logger = ProcessRunLogger::new(pool.clone()); | ||||||
| let worker_id = uuid::Uuid::new_v4(); | ||||||
|
|
||||||
| sqlx::query("INSERT INTO worker_runs (id, channel_id, status, result) VALUES (?, 'ch-1', 'running', '')") | ||||||
| .bind(worker_id.to_string()) | ||||||
| .execute(&pool) | ||||||
| .await | ||||||
| .expect("insert"); | ||||||
|
|
||||||
| logger.log_worker_cancelled(worker_id, "Worker cancelled: user requested"); | ||||||
| tokio::time::sleep(std::time::Duration::from_millis(50)).await; | ||||||
|
|
||||||
| let row = sqlx::query("SELECT status, result FROM worker_runs WHERE id = ?") | ||||||
| .bind(worker_id.to_string()) | ||||||
| .fetch_one(&pool) | ||||||
| .await | ||||||
| .expect("fetch"); | ||||||
| let status: String = sqlx::Row::try_get(&row, "status").expect("status"); | ||||||
| let result: String = sqlx::Row::try_get(&row, "result").expect("result"); | ||||||
| assert_eq!(status, "cancelled"); | ||||||
| assert_eq!(result, "Worker cancelled: user requested"); | ||||||
| } | ||||||
|
|
||||||
| #[tokio::test] | ||||||
| async fn cancel_running_detached_worker_does_not_touch_channel_bound_rows() { | ||||||
| let pool = setup_worker_runs_table().await; | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to persist
cancelledexplicitly. One thing to keep an eye on: the syntheticWorkerComplete { success: false }is still indistinguishable from a real failure for any downstream consumers that don't consultworker_runs.status(or parse the result text). If anything still mapssuccess=false=> failed, it may be worth threading a terminal status through the event shape in a follow-up.