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: worker name in job + better timeout handling for same_worker jobs #5248

Merged
merged 3 commits into from
Feb 8, 2025

Conversation

rubenfiszel
Copy link
Contributor

@rubenfiszel rubenfiszel commented Feb 8, 2025

Important

Improves timeout handling for same_worker jobs and assigns worker names in job processing, with UI and SQL query updates.

  • Behavior:
    • Improved timeout handling for same_worker jobs in monitor.rs by identifying long-dead workers and failing associated jobs.
    • Assigns worker name to jobs in windmill-queue/src/jobs.rs and windmill-worker/src/worker.rs.
  • SQL Queries:
    • Removed obsolete queries from .sqlx files.
    • Updated INSERT INTO v2_job_runtime queries to include ping column.
  • UI:
    • Updated HistoricInputs.svelte to display a message when job runs are limited to 5.

This description was created by Ellipsis for 0a42d29. It will automatically update as commits are pushed.

@rubenfiszel rubenfiszel requested review from HugoCasa and uael February 8, 2025 14:29
Copy link

cloudflare-workers-and-pages bot commented Feb 8, 2025

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: 0a42d29
Status: ✅  Deploy successful!
Preview URL: https://27dc5bf5.windmill.pages.dev
Branch Preview URL: https://rf-sameworker.windmill.pages.dev

View logs

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 423b0a1 in 2 minutes and 41 seconds

More details
  • Looked at 7784 lines of code in 214 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/lib/components/HistoricInputs.svelte:123
  • Draft comment:
    Changed from a
    to a with . Make sure the table semantic markup is correct inside the DataTable component and that this matches your design requirements.
  • Reason this comment was not posted:
    Marked as duplicate.
2. frontend/src/lib/components/HistoricInputs.svelte:123
  • Draft comment:
    Nice improvement: replacing the plain
    with a and makes the table body more semantically correct. However, note that using the condition jobs?.length == 5 to indicate a limited set might be too strict if the page limit changes; consider using the provided hasMoreCurrentRuns flag (or equivalent) to signal when additional runs exist.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_e2ccFXOLEie5t1XH


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 0a42d29 in 2 minutes and 8 seconds

More details
  • Looked at 12 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 14 drafted comments based on config settings.
1. backend/windmill-worker/src/worker_flow.rs:2653
  • Draft comment:
    Consider handling the error for the SQL update inside the if continue_on_same_worker { ... } block rather than discarding it with let _ = …. It may mask issues when updating the worker assignment.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. backend/windmill-worker/src/worker_flow.rs:3769
  • Draft comment:
    Usage of unreachable!("is simple flow") implies that other module variants cannot occur. Verify that all possible FlowModuleValue variants are handled; otherwise, this may hide a logic error if a new variant is added.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. backend/windmill-worker/src/worker_flow.rs:3640
  • Draft comment:
    Double-check that the conversion from the evaluated iterator expression into a Vec<Box> is robust, since deserialization may fail if the expression doesn't return an array. Consider improving error handling with a clearer message.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. backend/windmill-worker/src/worker_flow.rs:3920
  • Draft comment:
    In script_to_payload, the flow step timeout is determined by combining module and script timeout values. Ensure that the priority logic (module.timeout || script_timeout) is appropriate for all cases and that missing timeouts are handled correctly.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
5. backend/windmill-worker/src/worker_flow.rs:3981
  • Draft comment:
    The function from_now converts a Duration into a chrono DateTime. Review the behavior when the Duration exceeds the maximum allowed value; consider logging a warning or handling such cases explicitly.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. backend/windmill-worker/src/worker_flow.rs:3610
  • Draft comment:
    The is_simple_modules function uses several conditions to decide if a module is simple. Consider adding unit tests to verify that all desired constraints are met and that future changes in module structure will trigger test failures if they break the simplicity criteria.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
7. backend/windmill-worker/src/worker_flow.rs:2480
  • Draft comment:
    There is heavy usage of cloning (e.g. for passing around Arc/Marc structures). Consider reviewing if all clones are necessary, or if references can be passed more efficiently to reduce overhead.
  • Reason this comment was not posted:
    Confidence changes required: 40% <= threshold 50%
    None
8. backend/windmill-worker/src/worker_flow.rs:3540
  • Draft comment:
    The control flow in compute_next_flow_transform is complex and nested. Consider refactoring or breaking the logic into smaller helper functions for improved readability and maintainability.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. backend/windmill-worker/src/worker_flow.rs:1634
  • Draft comment:
    The 'push_next_flow_job' function spans several hundred lines and contains complex nested logic. Consider refactoring it into smaller, well-named helper functions for improved readability and maintainability.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. backend/windmill-worker/src/worker_flow.rs:3022
  • Draft comment:
    The recursive implementation of 'insert_iter_arg' may risk unbounded recursion if keys keep conflicting. Consider adding a maximum recursion limit or an alternative conflict resolution strategy.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. backend/windmill-worker/src/worker_flow.rs:3881
  • Draft comment:
    In 'script_to_payload', ensure that the two branches (when script_hash is none vs. present) handle permissions and on_behalf_of data consistently. Adding inline documentation here would improve clarity.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. backend/windmill-worker/src/worker_flow.rs:3951
  • Draft comment:
    The 'get_transform_context' function collects module results and constructs an IdContext. If the number of modules grows large, cloning all job results could pose performance concerns. Consider if lazy evaluation or borrowing could be applied.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. backend/windmill-worker/src/worker_flow.rs:3542
  • Draft comment:
    Functions like 'next_forloop_status' and 'next_loop_iteration' have intricate logic for handling iterators. Consider adding more inline documentation or breaking out parts of the logic into separate functions to aid future maintenance.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. backend/windmill-worker/src/worker_flow.rs:3755
  • Draft comment:
    Some JSON deserialization calls use unwrap (e.g. in 'payload_from_simple_module') without explicit error handling. Ensure that these unwraps are safe in production or replace them with proper error propagation.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_7mZbn6lCDaVP440X


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@rubenfiszel rubenfiszel merged commit 403826f into main Feb 8, 2025
8 checks passed
@rubenfiszel rubenfiszel deleted the rf/sameWorker branch February 8, 2025 21:20
@github-actions github-actions bot locked and limited conversation to collaborators Feb 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant