backend: migrate job tables to v2 schema (v2 phase 1)

[uael]

Important

Migrate job tables to a new v2 schema, updating database structure, SQL queries, and codebase to support the new schema.

• Database Migrations:
  • Add new tables v2_job_runtime, v2_job_status, v2_job_queue, and v2_job_completed with associated policies and grants.
  • Create views for backward compatibility with old table names (queue, completed_job, etc.).
  • Add triggers to sync data between v1 and v2 tables.
  • Add indices for performance optimization on v2_job and v2_job_completed.
• Code Changes:
  • Update SQL queries in monitor.rs, jobs.rs, worker.rs, and worker_flow.rs to use new v2 tables.
  • Modify functions to handle new schema, including handle_dependency_job, handle_flow_dependency_job, and handle_app_dependency_job.
  • Adjust tests in worker.rs to align with schema changes.
• Miscellaneous:
  • Update base.sql and hello.sql fixtures to reflect new schema.
  • Add new test cases for job labels and script hash payloads with preprocessors.

^{This description was created by for 56653fd. It will automatically update as commits are pushed.}

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	56653fd
Status:	✅  Deploy successful!
Preview URL:	https://f65bb535.windmill.pages.dev
Branch Preview URL:	https://uael-v2-phase-1.windmill.pages.dev

View logs

[ellipsis-dev]

👍 Looks good to me! Incremental review on c1b63cc in 1 minute and 11 seconds

More details

• Looked at 171 lines of code in 5 files
• Skipped 0 files when reviewing.
• Skipped posting 4 drafted comments based on config settings.

1. backend/migrations/20250117124431_v2_job.up.sql:2

• Draft comment:
  Ensure the script_lang type is created before using it in the v2_job table. Add a CREATE TYPE statement for script_lang if it doesn't already exist.
• Reason this comment was not posted:
  Comment did not seem useful.

2. backend/migrations/20250117124743_v2_job_queue_sync.up.sql:21

• Draft comment:
  Ensure consistent handling of trigger and trigger_kind across the codebase to avoid confusion and potential data integrity issues. Consider renaming trigger to something more descriptive if it stores different types of data based on trigger_kind.
• Reason this comment was not posted:
  Comment did not seem useful.

3. backend/migrations/20250117124744_v2_job_completed_sync.up.sql:25

• Draft comment:
  Ensure consistent handling of trigger and trigger_kind across the codebase to avoid confusion and potential data integrity issues. Consider renaming trigger to something more descriptive if it stores different types of data based on trigger_kind.
• Reason this comment was not posted:
  Marked as duplicate.

4. backend/migrations/20250117124748_v2_migrate_from_v1.up.sql:28

• Draft comment:
  Ensure consistent handling of trigger and trigger_kind across the codebase to avoid confusion and potential data integrity issues. Consider renaming trigger to something more descriptive if it stores different types of data based on trigger_kind.
• Reason this comment was not posted:
  Marked as duplicate.

Workflow ID: wflow_kPWJqOF6pxzleXmz

---

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

[ellipsis-dev]

👍 Looks good to me! Incremental review on 5213a15 in 40 seconds

More details

• Looked at 2857 lines of code in 74 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. backend/windmill-worker/src/worker_lockfiles.rs:642

• Draft comment:
  Consider using v.unwrap_or(false) instead of Some(true) == v for clarity and idiomatic Rust.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The code uses sqlx::query_scalar! to fetch a boolean value, but the result is compared using Some(true) == v, which is not idiomatic. It would be clearer to use v.unwrap_or(false) directly.

Workflow ID: wflow_zapveXQZvNJYhNdm

---

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

[ellipsis-dev]

👍 Looks good to me! Incremental review on dec7a72 in 48 seconds

More details

• Looked at 2857 lines of code in 74 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. backend/windmill-worker/src/worker_lockfiles.rs:1531

• Draft comment:
  Consider logging or handling conflicts when using ON CONFLICT DO NOTHING to ensure data integrity and traceability.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The code uses ON CONFLICT DO NOTHING in SQL queries, which is a common pattern to avoid errors when inserting duplicate entries. However, it might be beneficial to log or handle these conflicts in some way to ensure data integrity and traceability.

Workflow ID: wflow_2p0371nQZYhVLZeD

---

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

[ellipsis-dev]

👍 Looks good to me! Incremental review on a1e0868 in 29 seconds

More details

• Looked at 2857 lines of code in 74 files
• Skipped 0 files when reviewing.
• Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_vxPWpbic8nEhXLON

---

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

[ellipsis-dev]

👍 Looks good to me! Incremental review on 2df13f5 in 36 seconds

More details

• Looked at 1836 lines of code in 50 files
• Skipped 0 files when reviewing.
• Skipped posting 6 drafted comments based on config settings.

1. backend/.sqlx/query-036c84bb9ce72748956bc9c18fbe276444fab025a281dc4784596b0e31c1cb9d.json:3

• Draft comment:
  Ensure that the index ix_job_workspace_id_created_at_new_9 is necessary and does not overlap with other indexes on v2_job to avoid redundant indexes and resource usage.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The PR involves creating multiple indexes concurrently on the v2_job table. It's important to ensure that these indexes are necessary and do not overlap in functionality, which could lead to unnecessary resource usage.

2. backend/.sqlx/query-5401c521b5e63b7d9e7bc51c19d116599f6bcedbe70f3bf346b482fe79501958.json:3

• Draft comment:
  Ensure that the index ix_job_workspace_id_created_at_new_8 is necessary and does not overlap with other indexes on v2_job to avoid redundant indexes and resource usage.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The PR involves creating multiple indexes concurrently on the v2_job table. It's important to ensure that these indexes are necessary and do not overlap in functionality, which could lead to unnecessary resource usage.

3. backend/.sqlx/query-8be277b89102a26dda506202a3ef7eb05342cfb3aa9b4f5d80c70fbc50d437ba.json:3

• Draft comment:
  Ensure that the index ix_job_workspace_id_created_at_new_6 is necessary and does not overlap with other indexes on v2_job to avoid redundant indexes and resource usage.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The PR involves creating multiple indexes concurrently on the v2_job table. It's important to ensure that these indexes are necessary and do not overlap in functionality, which could lead to unnecessary resource usage.

4. backend/.sqlx/query-ae8dfecd46425d5f86003eea9a578e9831fc0e700cc76ab9627afe9040a4efe0.json:3

• Draft comment:
  Ensure that the index ix_job_workspace_id_created_at_new_7 is necessary and does not overlap with other indexes on v2_job to avoid redundant indexes and resource usage.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The PR involves creating multiple indexes concurrently on the v2_job table. It's important to ensure that these indexes are necessary and do not overlap in functionality, which could lead to unnecessary resource usage.

5. backend/.sqlx/query-c7be5fa2eaf66147c1213046e615f5e9fd168ef1e3aba8af64b15341055d6007.json:3

• Draft comment:
  Ensure that the index ix_job_workspace_id_created_at_new_5 is necessary and does not overlap with other indexes on v2_job to avoid redundant indexes and resource usage.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The PR involves creating multiple indexes concurrently on the v2_job table. It's important to ensure that these indexes are necessary and do not overlap in functionality, which could lead to unnecessary resource usage.

6. backend/.sqlx/query-f64ae18811e211dbf0cb98b43d3b018b0dcc0abc7e4a1f0b45885cfe18efd9b2.json:3

• Draft comment:
  Ensure that the index ix_job_workspace_id_created_at_new_3 is necessary and does not overlap with other indexes on v2_job to avoid redundant indexes and resource usage.
• Reason this comment was not posted:
  Confidence changes required: 50%
  The PR involves creating multiple indexes concurrently on the v2_job table. It's important to ensure that these indexes are necessary and do not overlap in functionality, which could lead to unnecessary resource usage.

Workflow ID: wflow_oqR2JvoNI5bh0prw

---

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

[ellipsis-dev]

❌ Changes requested. Incremental review on a45c359 in 1 minute and 54 seconds

More details

• Looked at 143 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. backend/tests/worker.rs:3768

• Draft comment:
  Nice reuse of a closure to run the test with different original label sets. For clarity it might be helpful to give this closure a more descriptive name (e.g. run_label_test) rather than just 'test'.
• Reason this comment was not posted:
  Confidence changes required: 30%
  None

2. backend/tests/worker.rs:3761

• Draft comment:
  Consider explicitly documenting deduplication of job labels. The test expects labels to be formed by chaining original labels with ["yolo", "greet", "Jean Neige"], which implies duplicate entries (like "greet") are deduplicated. Clarifying this behavior would improve readability.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  While the comment correctly identifies that deduplication is happening, this appears to be an internal implementation detail rather than a user-facing feature that requires documentation. The test is focused on verifying the basic functionality of job labels working correctly. Deduplication is likely handled automatically by the database schema or internal processing, and doesn't need to be explicitly documented.
  The comment does identify a real behavior in the code. Perhaps deduplication is an important feature that users should know about when working with job labels?
  While deduplication exists, it's an implementation detail rather than a core feature. Users just need to know they can add labels - the fact that duplicates are removed is not something they need to actively think about or work around. Over-documenting internal details can make code harder to maintain.
  Delete the comment. The deduplication behavior is an implementation detail that doesn't need explicit documentation. The test adequately verifies the behavior without needing additional documentation.

3. backend/tests/worker.rs:3805

• Draft comment:
  The test module is very large and covers a wide range of functionalities (multiple languages, flows, dependencies, schedule handlers, etc.). Consider splitting it into smaller, well‐organized modules and using common helper functions to improve maintainability and reusability.
• Reason this comment was not posted:
  Comment was on unchanged code.

Workflow ID: wflow_LLzP04tLFkqlabZG

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 6ce80c1 in 1 minute and 15 seconds

More details

• Looked at 145 lines of code in 5 files
• Skipped 0 files when reviewing.
• Skipped posting 10 drafted comments based on config settings.

1. backend/.sqlx/query-9d518842a9ad90ff9c28dc39690deb0ee6b62cf1d8ae1a02b28c23255d377b3d.json:1

• Draft comment:
  Query looks good; ensure 'labels' as TextArray is expected nullable.
• Reason this comment was not posted:
  Confidence changes required: 0%
  None

2. backend/.sqlx/query-9db563de47401fdfbedecbc52ec07aa5e03ec3adb1312b016639c3b4daa78bc5.json:1

• Draft comment:
  Formatting with newline is acceptable; verify consistency if intentional.
• Reason this comment was not posted:
  Confidence changes required: 0%
  None

3. backend/.sqlx/query-a17034c6c588162726b73c065e8e2995cfbeeafdd344f0c8b0ae863e1072ea40.json:1

• Draft comment:
  This query duplicates the one above; confirm both variants are needed.
• Reason this comment was not posted:
  Comment did not seem useful.

4. backend/.sqlx/query-bd5a0c06e2f2361c9fc670eb0b975b58d65ca93d68b29124d04bd526239b9df2.json:1

• Draft comment:
  Ensure the update condition with '$2::TEXT[] IS NOT NULL' is intentional to skip null updates.
• Reason this comment was not posted:
  Confidence changes required: 33%
  None

5. backend/.sqlx/query-cd5f02cf10cbf92dd1df53a54f2110efa11a7731ad0f0e5509f55efabdf535cd.json:1

• Draft comment:
  Simple select query; no issues found.
• Reason this comment was not posted:
  Confidence changes required: 0%
  None

6. backend/.sqlx/query-9d518842a9ad90ff9c28dc39690deb0ee6b62cf1d8ae1a02b28c23255d377b3d.json:7

• Draft comment:
  Query for 'labels' looks good. Confirm that 'nullable': true is intended when no labels are assigned.
• Reason this comment was not posted:
  Confidence changes required: 0%
  None

7. backend/.sqlx/query-9db563de47401fdfbedecbc52ec07aa5e03ec3adb1312b016639c3b4daa78bc5.json:3

• Draft comment:
  Consider normalizing whitespace formatting in the query (extra spaces before FROM clause).
• Reason this comment was not posted:
  Confidence changes required: 33%
  None

8. backend/.sqlx/query-a17034c6c588162726b73c065e8e2995cfbeeafdd344f0c8b0ae863e1072ea40.json:2

• Draft comment:
  This query is nearly identical to the one in query-9db563… file (aside from whitespace). Consider consolidating if both are not needed.
• Reason this comment was not posted:
  Marked as duplicate.

9. backend/.sqlx/query-bd5a0c06e2f2361c9fc670eb0b975b58d65ca93d68b29124d04bd526239b9df2.json:3

• Draft comment:
  Verify that the condition '$2::TEXT[] IS NOT NULL' is intentional, as it prevents updating labels to NULL.
• Reason this comment was not posted:
  Comment did not seem useful.

10. backend/.sqlx/query-cd5f02cf10cbf92dd1df53a54f2110efa11a7731ad0f0e5509f55efabdf535cd.json:2

• Draft comment:
  SELECT query for 'preprocessed' is straightforward; no issues found.
• Reason this comment was not posted:
  Confidence changes required: 0%
  None

Workflow ID: wflow_YNJhjSAIO5RUG0lx

---

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

[ellipsis-dev]

👍 Looks good to me! Incremental review on abfb5f5 in 1 minute and 13 seconds

More details

• Looked at 30 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 8 drafted comments based on config settings.

1. backend/tests/worker.rs:3767

• Draft comment:
  The test_job_labels function uses raw SQL and manual label sorting. Consider adding comments to clarify the intended labels transformation and confirm that duplicate labels (like "greet") are handled intentionally.
• 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.

2. backend/tests/worker.rs:3761

• Draft comment:
  This test file is very extensive and contains many almost-duplicated patterns across language integrations and payload types. Consider refactoring or splitting into submodules/files for improved maintainability and 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.

3. backend/tests/worker.rs:3885

• Draft comment:
  Many tests use multiple unwrap() calls. While acceptable in test code, using expect() with informative messages could aid in debugging failures.
• 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/tests/worker.rs:3820

• Draft comment:
  The test_job_labels function compares the database-returned labels to a sorted expected list. It is assumed that the labels stored in v2_job are sorted. It might be more robust to explicitly sort the fetched labels before comparing to avoid ordering issues.
• Reason this comment was not posted:
  Comment was on unchanged code.

5. backend/tests/worker.rs:100

• Draft comment:
  This test file is very large with many inline script strings and JSON literals. Consider extracting common scripts or JSON fixtures into constants/files to improve maintainability and readability.
• Reason this comment was not posted:
  Confidence changes required: 50%
  None

6. backend/tests/worker.rs:3837

• Draft comment:
  The version flags are managed using lazy_static with Arc<RwLock>. For modern code clarity consider using 'once_cell' or other patterns to reduce boilerplate.
• Reason this comment was not posted:
  Confidence changes required: 30%
  None

7. backend/tests/worker.rs:4135

• Draft comment:
  There are many unwrap() calls throughout the tests. While acceptable in a testing context, adding error messages can help diagnose failures quickly.
• Reason this comment was not posted:
  Confidence changes required: 30%
  None

8. backend/tests/worker.rs:3447

• Draft comment:
  The integration tests are very comprehensive. However, consider splitting this huge file into multiple test modules (e.g., job labels, script languages, schedule tests) to improve modularity and ease maintainability.
• Reason this comment was not posted:
  Confidence changes required: 40%
  None

Workflow ID: wflow_23Ll0hdZM8b4Aecd

---

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

[uael]

@HugoCasa

[ellipsis-dev]

👍 Looks good to me! Incremental review on 013d112 in 1 minute and 4 seconds

More details

• Looked at 28 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 11 drafted comments based on config settings.

1. backend/tests/worker.rs:3924

• Draft comment:
  This test file is extremely long and covers many cases. Consider splitting or modularizing some of these tests to ease 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.

2. backend/tests/worker.rs:3940

• Draft comment:
  Multiple tests use hard-coded magic numbers (e.g. version numbers like 1443253234253454) and literal strings for schedules. Consider extracting these into named constants for 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.

3. backend/tests/worker.rs:3189

• Draft comment:
  Many tests are using direct unwrap and expect calls. While acceptable in tests, adding more context to errors (via .expect with messages) can help debugging failures.
• 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/tests/worker.rs:3197

• Draft comment:
  Timeout values (e.g. Duration::from_millis(5000)) used in scheduled job tests could be potentially brittle under load. Consider reviewing these timeout durations for stability.
• 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.

5. backend/tests/worker.rs:3920

• Draft comment:
  The migration tests now reference the new v2 schema (e.g., queries on v2_job). Ensure that these tests are isolated and that any schema-specific SQL (like JSON extraction on the 'result' column) remains robust across PostgreSQL versions.
• 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/tests/worker.rs:4260

• Draft comment:
  There is significant duplication in test patterns (e.g., for flows, dependencies, script hash payloads, etc.). Consider refactoring repetitive patterns into helper functions to reduce code duplication and ease future changes.
• 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.

7. backend/tests/worker.rs:3925

• Draft comment:
  There’s a TODO regarding checking _metadata in flow_status (v2 phase 4). Ensure you either add a clear migration plan or remove the temporary check once v2 is finalized.
• 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.

8. backend/tests/worker.rs:4240

• Draft comment:
  Magic version numbers (e.g. 1443253234253454) appear in multiple tests. Consider replacing them with a named constant to improve clarity and ease 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.

9. backend/tests/worker.rs:3828

• Draft comment:
  The helper function 'test_for_versions' is very useful for iterative testing over version flags. Adding some inline documentation on its purpose and usage would improve code readability.
• Reason this comment was not posted:
  Confidence changes required: 30%
  None

10. backend/tests/worker.rs:3596

• Draft comment:
  Tests covering relative imports for Bun, Deno, and Python are thorough. To reduce duplication, consider parameterizing these tests or factoring common logic into shared helper functions.
• Reason this comment was not posted:
  Confidence changes required: 40%
  None

11. backend/tests/worker.rs:3768

• Draft comment:
  While using unwrap() is acceptable in tests, providing context messages (or using expect with descriptive errors) can improve debugging when tests fail.
• Reason this comment was not posted:
  Confidence changes required: 30%
  None

Workflow ID: wflow_aKwwhJ6cOdoA65wB

---

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

[ellipsis-dev]

👍 Looks good to me! Incremental review on 7519a8b in 49 seconds

More details

• Looked at 3392 lines of code in 79 files
• Skipped 0 files when reviewing.
• Skipped posting 9 drafted comments based on config settings.

1. backend/windmill-worker/src/worker_lockfiles.rs:163

• Draft comment:
  Avoid using unwrap() when calling to_str() on the normalized path. Consider handling the None case to avoid panics when non-UTF8 paths are encountered.
• 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.

2. backend/windmill-worker/src/worker_lockfiles.rs:99

• Draft comment:
  Consider clarifying the SQL query cast syntax in dependency map insertion; embedding $4::text::IMPORTER_KIND might benefit from a comment or refactoring for readability.
• Reason this comment was not posted:
  Confidence changes required: 50%
  None

3. backend/windmill-worker/src/worker_lockfiles.rs:244

• Draft comment:
  Check handling of errors when canonicalizing relative import paths. Instead of logging an error with unwrap() (line 197), consider propagating the error or handling it gracefully.
• 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_lockfiles.rs:928

• Draft comment:
  Box::pin is used when calling async functions like lock_modules within reduce_flow. Consider if it is necessary, as excessive boxing may add overhead. A plain .await might suffice.
• Reason this comment was not posted:
  Confidence changes required: 50%
  None

5. backend/windmill-worker/src/worker_lockfiles.rs:1307

• Draft comment:
  In skip_creating_new_lock, the logic for TypeScript annotations switch between Bun and Bunnative might be error-prone if annotations change; ensure tests cover these edge cases.
• 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_lockfiles.rs:163

• Draft comment:
  Avoid using unwrap() on to_str() in 'try_normalize'. Consider propagating an error instead of panicking on non-UTF8 paths.
• 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.

7. backend/windmill-worker/src/worker_lockfiles.rs:790

• Draft comment:
  Consider using #[async_recursion] on the 'lock_modules' function to avoid manually boxing recursive async calls with Box::pin.
• 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.

8. backend/windmill-worker/src/worker_lockfiles.rs:1267

• Draft comment:
  In 'reduce_app', the inlineScript branch unwraps 'content' without graceful error handling. Consider propagating an error if 'content' is missing.
• 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_lockfiles.rs:1

• Draft comment:
  This file is quite large and complex. Consider refactoring into smaller modules to improve maintainability and readability.
• 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_BAaHepeEC0NVp753

---

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

[ellipsis-dev]

👍 Looks good to me! Incremental review on a16f275 in 37 seconds

More details

• Looked at 279 lines of code in 8 files
• Skipped 0 files when reviewing.
• Skipped posting 11 drafted comments based on config settings.

1. backend/tests/worker.rs:5

• Draft comment:
  Multiple unwrap() calls are used throughout tests. While acceptable in tests, consider adding contextual error messages for clarity when failures occur.
• 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.

2. backend/tests/worker.rs:1020

• Draft comment:
  The worker spawning logic (e.g., in spawn_test_worker) is duplicated and complex. Consider refactoring this into a reusable helper with clear documentation to improve 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.

3. backend/tests/worker.rs:3200

• Draft comment:
  Fixed timeout durations (e.g., 5000 ms) in asynchronous test waits may lead to flakiness in CI. Consider parameterizing these or adding retries to improve test stability.
• 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/tests/worker.rs:3828

• Draft comment:
  The test_for_versions helper, which iterates over version flags, is a bit complex. Adding inline comments to clarify its purpose and expected behavior would help future maintainers.
• 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.

5. backend/tests/worker.rs:3745

• Draft comment:
  When executing queries that update or fetch labels (e.g., in test_job_labels), ensure that order or duplicate values are handled consistently. Document assumptions about label sorting if order matters.
• 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/tests/worker.rs:980

• Draft comment:
  Refactor common worker initialization logic in functions like spawn_test_worker and in_test_worker. This repetition makes maintenance harder.
• 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.

7. backend/tests/worker.rs:80

• Draft comment:
  Consider moving global test setup (e.g. initialize_tracing and JWT secret setup) into a shared test fixture to avoid redundant calls.
• 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.

8. backend/tests/worker.rs:2420

• Draft comment:
  Avoid overuse of unwrap() in tests; consider using expect with descriptive error messages or proper error propagation to ease debugging.
• 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/tests/worker.rs:3547

• Draft comment:
  The functions run_deployed_relative_imports and run_preview_relative_imports share similar structure. Consider consolidating them to reduce duplication.
• 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/tests/worker.rs:3880

• Draft comment:
  The test file is very long and covers numerous scenarios. Splitting tests into multiple modules or files could improve 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.

11. backend/tests/worker.rs:4250

• Draft comment:
  Consider adding brief documentation or inline comments to explain the purpose of complex test helpers like run_until_complete_with.
• Reason this comment was not posted:
  Confidence changes required: 50%
  None

Workflow ID: wflow_qq5PUgqoL9XZodHR

---

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

[ellipsis-dev]

❌ Changes requested. Incremental review on e9d0c72 in 1 minute and 16 seconds

More details

• Looked at 46 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 5 drafted comments based on config settings.

1. backend/migrations/20250201124743_v2_job_queue_sync.up.sql:160

• Draft comment:
  Consider adding an ELSE clause to preserve the current value of 'preprocessed'. This avoids setting it to NULL.
• Reason this comment was not posted:
  Confidence changes required: 0%
  None

2. backend/migrations/20250201124744_v2_job_completed_sync.up.sql:77

• Draft comment:
  Double-check the business logic change from checking 'script_entrypoint_override' to 'job.kind'. Ensure this condition matches expected behavior.
• Reason this comment was not posted:
  Confidence changes required: 33%
  None

3. backend/migrations/20250201124743_v2_job_queue_sync.up.sql:160

• Draft comment:
  Using ELSE preprocessed preserves the value when not FALSE. Confirm this is the intended logic.
• Reason this comment was not posted:
  Confidence changes required: 10%
  None

4. backend/migrations/20250201124744_v2_job_completed_sync.up.sql:77

• Draft comment:
  Changing the condition from checking script_entrypoint_override to job.kind alters the semantics. Verify that 'job.kind = "script"' correctly captures the intended scenario.
• Reason this comment was not posted:
  Confidence changes required: 50%
  None

5. backend/migrations/20250201124744_v2_job_completed_sync.up.sql:146

• Draft comment:
  The v2_job_completed_before_update trigger function is missing a RETURN NEW statement. All BEFORE triggers must return NEW.
• Reason this comment was not posted:
  Marked as duplicate.

Workflow ID: wflow_LRNW1W3yVDsaXdE0

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on f1d3850 in 1 minute and 26 seconds

More details

• Looked at 20 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 4 drafted comments based on config settings.

1. backend/migrations/20250201124744_v2_job_completed_sync.up.sql:146

• Draft comment:
  Ensure that returning NULL in the BEFORE UPDATE trigger is intentional, as it cancels the row update. If you want the update to apply changes to v2_job_completed, consider returning NEW.
• Reason this comment was not posted:
  Comment did not seem useful.

2. backend/migrations/20250201124744_v2_job_completed_sync.down.sql:3

• Draft comment:
  Dropping the v2_job_completed_before_update function appears correct. Just double-check dependencies to ensure no unintended side effects.
• Reason this comment was not posted:
  Confidence changes required: 33%
  None

3. backend/migrations/20250201124744_v2_job_completed_sync.down.sql:3

• Draft comment:
  Consider using DROP FUNCTION IF EXISTS to avoid errors if the function is already absent.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  Migration scripts should be deterministic and idempotent. However, down migrations are typically only run once to revert a specific change. IF EXISTS is more relevant for idempotent scripts or general maintenance scripts, not for specific version migrations. The function should definitely exist when running this down migration, as it was created in the corresponding up migration.
  IF EXISTS could make the script more robust against edge cases where the function was already manually dropped. It's a defensive programming practice.
  In a versioned migration system, we expect exact state transitions. Adding IF EXISTS could mask real problems and make debugging harder if something is unexpectedly missing.
  The comment should be deleted. Using IF EXISTS here would add unnecessary complexity and could hide real issues in the migration process.

4. backend/migrations/20250201124744_v2_job_completed_sync.up.sql:146

• Draft comment:
  Returning NULL in a BEFORE UPDATE trigger cancels the update on v2_job_completed. Confirm if this suppression is intended.
• Reason this comment was not posted:
  Marked as duplicate.

Workflow ID: wflow_EQ5Y6Mrl8Q4UlrDX

---

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

[ellipsis-dev]

👍 Looks good to me! Incremental review on aa95bb4 in 2 minutes and 17 seconds

More details

• Looked at 3506 lines of code in 81 files
• Skipped 0 files when reviewing.
• Skipped posting 12 drafted comments based on config settings.

1. backend/windmill-worker/src/worker_lockfiles.rs:1307

• Draft comment:
  The function 'skip_creating_new_lock' uses an inverted condition which may confuse; consider renaming or adding a comment for 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.

2. backend/windmill-worker/src/worker_lockfiles.rs:184

• Draft comment:
  Repeated SQL cast notation could be refactored for clarity by extracting constants or helper macros.
• 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_lockfiles.rs:894

• Draft comment:
  Recursive calls with Box::pin in 'lock_modules' may risk stack overflow; consider refactoring to iterative approach.
• 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_lockfiles.rs:637

• Draft comment:
  The cancellation check using fetch_optional silently returns false on error; consider propagating the error if appropriate.
• Reason this comment was not posted:
  Comment was on unchanged code.

5. backend/windmill-worker/src/worker_lockfiles.rs:1256

• Draft comment:
  The 'reduce_flow' function is lengthy and complex; splitting it into smaller helper functions with documentation could improve 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.

6. backend/windmill-worker/src/worker_lockfiles.rs:160

• Draft comment:
  The function try_normalize returns None for Prefix/RootDir components. Consider returning a more descriptive error (or a Result) to aid debugging when a path cannot be normalized.
• 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.

7. backend/windmill-worker/src/worker_lockfiles.rs:1308

• Draft comment:
  The skip_creating_new_lock function’s logic is a bit confusing due to its naming and conditional behavior. It may be clearer to rename it (e.g., should_create_new_lock) or add inline comments explaining when it returns true versus false.
• 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.

8. backend/windmill-worker/src/worker_lockfiles.rs:1164

• Draft comment:
  In reduce_flow, module.value is repeatedly deserialized from JSON. It might improve performance and clarity to cache or memoize the parsed value to avoid duplicate work.
• 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_lockfiles.rs:230

• Draft comment:
  In handle_dependency_job, after updating the script lock and invalidating the cache, errors from handle_deployment_metadata are only logged. Consider whether deployment metadata failures should be propagated or retried rather than silently logged.
• 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_lockfiles.rs:416

• Draft comment:
  In trigger_dependents_to_recompute_dependencies, the query uses array_agg on importer_node_id. Please confirm that the aggregation handles NULL values correctly and that the resulting array is handled properly downstream.
• 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_lockfiles.rs:760

• Draft comment:
  The recursive locking functions (lock_modules and lock_modules_app) have complex control flow. Adding more inline comments explaining each branch would improve maintainability and ease future debugging.
• 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_lockfiles.rs:1668

• Draft comment:
  In capture_dependency_job, ensure that each branch for different ScriptLang variants (e.g., Python3, Bun, Php, etc.) properly handles edge cases and malformed data. Additional tests for these branches may help avoid silent failures.
• 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_QFEnzgwoUAHGUuJU

---

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

[ellipsis-dev]

👍 Looks good to me! Incremental review on 6dc830c in 55 seconds

More details

• Looked at 3506 lines of code in 81 files
• Skipped 0 files when reviewing.
• Skipped posting 7 drafted comments based on config settings.

1. backend/windmill-worker/src/worker_lockfiles.rs:189

• Draft comment:
  Avoid using unwrap() after try_normalize. Use a safe conversion (e.g. expect with a descriptive message) to convert PathBuf to string, so that an unexpected non-UTF8 path won’t panic.
• 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.

2. backend/windmill-worker/src/worker_lockfiles.rs:760

• Draft comment:
  The lock_modules function is very complex. Consider refactoring or adding more inline comments to improve 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.

3. backend/windmill-worker/src/worker_lockfiles.rs:189

• Draft comment:
  Consider handling non‐UTF8 paths gracefully in try_normalize. Currently, when calling to_str().unwrap() (line 189–195) in parse_bun_relative_imports, the code may panic if the normalized path isn’t valid UTF‑8.
• 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_lockfiles.rs:765

• Draft comment:
  In the lock_modules function, e.get_value() is called more than once. Storing the result of e.get_value()? in a local variable would avoid redundant computation and potential inconsistency if the value changes.
• 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.

5. backend/windmill-worker/src/worker_lockfiles.rs:1307

• Draft comment:
  The skip_creating_new_lock function’s logic is not immediately clear. Adding a comment describing why locks are not recreated for certain Bun/Bunnative conditions would improve readability.
• 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_lockfiles.rs:66

• Draft comment:
  Transaction handling in functions like update_script_dependency_map and later in lock_modules uses successive reassignments of tx (e.g. lines 66–81, 142–158). Consider refactoring these to reduce chaining of transaction reassignments for clarity and to reduce error‐proneness.
• 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.

7. backend/windmill-worker/src/worker_lockfiles.rs:1160

• Draft comment:
  The file is very long and bundles many responsibilities (dependency map updates, lockfile generation, flow locking, app dependency handling, etc.). Consider splitting it into multiple modules based on functionality to improve code 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.

Workflow ID: wflow_PYx5SESCY6kbLgHx

---

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

[ellipsis-dev]

👍 Looks good to me! Incremental review on 56653fd in 1 minute and 24 seconds

More details

• Looked at 3499 lines of code in 81 files
• Skipped 0 files when reviewing.
• Skipped posting 9 drafted comments based on config settings.

1. backend/windmill-worker/src/worker_lockfiles.rs:1308

• Draft comment:
  The function skip_creating_new_lock always returns true if none of the special conditions for Bun/Bunnative are met. It might be worth clarifying the intent here – should other languages always skip creating a new lock, or is this function intended only for Bun/Bunnative scripts?
• 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.

2. backend/windmill-worker/src/worker_lockfiles.rs:163

• Draft comment:
  In try_normalize, encountering a RootDir or Prefix immediately returns None. Ensure this behavior is intended, since some absolute paths might be valid in your domain. Consider adding a comment to explain the rationale if appropriate.
• Reason this comment was not posted:
  Confidence changes required: 50%
  None

3. backend/windmill-worker/src/worker_lockfiles.rs:889

• Draft comment:
  The use of Box::pin(lock_modules(...)).await inside several match arms (e.g. in lock_modules) increases complexity. Consider refactoring these recursive calls into smaller helper functions for increased clarity 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.

4. backend/windmill-worker/src/worker_lockfiles.rs:1164

• Draft comment:
  The function reduce_flow iterates and recursively processes modules. Its complexity is high. Consider splitting it up or adding detailed inline comments describing the transformation logic to improve readability.
• 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.

5. backend/windmill-worker/src/worker_lockfiles.rs:189

• Draft comment:
  Consider using Rust’s Path API (e.g. Path::join or PathBuf::push) instead of string formatting to compute relative paths, for increased robustness.
• 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_lockfiles.rs:160

• Draft comment:
  If normalization fails (returns None), log a warning to aid debugging instead of failing silently.
• 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.

7. backend/windmill-worker/src/worker_lockfiles.rs:765

• Draft comment:
  The lock_modules function is complex. Consider refactoring by extracting helper functions to simplify the nested async calls and improve 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.

8. backend/windmill-worker/src/worker_lockfiles.rs:1167

• Draft comment:
  Add a comment explaining why replacing the module value with 'Identity' is safe and what it signifies.
• 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_lockfiles.rs:414

• Draft comment:
  Improve error handling in trigger_dependents_to_recompute_dependencies by adding more detailed logging if expected importer information is missing.
• 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_caohyKdnFi1rUHJ0

---

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