refactor: move concurrency and debouncing settings to their own structs

[pyranota]

No description provided.

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	829ecef
Status:	✅  Deploy successful!
Preview URL:	https://32fc2cc8.windmill.pages.dev
Branch Preview URL:	https://debouncing-and-concurrency-s.windmill.pages.dev

View logs

[pyranota]

/eeref

[windmill-internal-app]

Starting ee ref update...

View workflow run

[windmill-internal-app]

Successfully updated ee-repo-ref.txt

[claude]

Claude finished @pyranota's task —— View job

---

PR Review Complete ✅

I've completed a comprehensive review of PR #7286. This is a solid refactoring that improves code organization by separating concurrency and debouncing settings into their own structs.

---

Summary

This PR refactors concurrency and debouncing settings from scattered individual fields into dedicated ConcurrencySettings and DebouncingSettings structs. The changes span 15 files with +653/-775 lines, resulting in cleaner, more maintainable code.

Overall Assessment: ✅ Approve - This is a well-executed refactoring with good backwards compatibility handling.

---

Positive Aspects

1. Better Type Safety: Moving from Option<T> to non-optional structs with internal Option fields provides clearer semantics - settings always exist, but individual values within may be None.

2. Improved Code Organization: Grouping related settings (concurrency_key, concurrent_limit, concurrency_time_window_s) into a single struct makes the codebase more maintainable and reduces parameter coupling.

3. Backwards Compatibility: The use of #[serde(alias = "custom_debounce_key")] ensures older API clients continue to work.

4. Consistent Defaults: Using Default trait and .is_default() helper provides consistent initialization patterns.

5. Reduced Code Duplication: The refactoring eliminated ~122 lines of code overall, suggesting good consolidation.

---

Issues & Recommendations

🟡 Medium Priority Issues

1. Inconsistent Naming Pattern (backend/windmill-common/src/jobs.rs:529-535)

   The struct has both ConcurrencySettings and ConcurrencySettingsWithCustom. While I understand WithCustom is for API deserialization with the custom_concurrency_key field name, this naming pattern is confusing.

   Recommendation: Add clear doc comments explaining when to use each:

   /// Internal representation of concurrency settings used in job processing
   pub struct ConcurrencySettings { ... }
   
   /// API/serialization representation with 'custom_' prefix for backwards compatibility
   pub struct ConcurrencySettingsWithCustom { ... }

2. Commented-Out Code (backend/windmill-common/src/jobs.rs:460)

   DeploymentCallback {
       path: String,
       // debouncing_settings: Option<DebouncingSettings>,
   },

   Recommendation: Either remove the commented field entirely or add a TODO comment explaining why debouncing was intentionally removed from DeploymentCallback.

3. Incomplete TODO Comments (backend/windmill-common/src/jobs.rs:498-507)

   /// Top threshhold on max delay
   /// TODO(claude): explain better, in 3 lines tops
   pub max_total_time: Option<i32>,
   
   /// Top threshold on amount of debounces
   /// TODO(claude): explain better, in 3 lines tops
   pub max_total_amount: Option<i32>,

   Recommendation: Either complete these documentation TODOs or create a follow-up issue to document these fields properly. The "TODO(claude)" prefix suggests these were meant for me to complete!

4. Test Import Oddity (backend/windmill-api/src/flows.rs:1557)

   use aws_sdk_config::operation::stop_configuration_recorder::builders::StopConfigurationRecorderOutputBuilder;

   This AWS SDK import appears in the test module but is never used in the visible test code.

   Recommendation: Remove if unused, or add a comment explaining its purpose.

🟢 Minor/Style Issues

5. Verbose Destructuring (backend/windmill-queue/src/jobs.rs)

   The refactoring introduces verbose destructuring patterns:

   let ConcurrencySettings { 
       mut concurrency_key,
       mut concurrent_limit,
       concurrency_time_window_s,
   } = concurrency_settings;

   Note: This is acceptable, but consider if accessor methods might be cleaner in heavily-used paths.

6. Serialization Asymmetry (backend/windmill-common/src/jobs.rs:584)

   RawCode is Deserialize but not Serialize:

   #[derive(Clone, Deserialize, Debug, Default)]
   pub struct RawCode {

   Recommendation: If this is intentional (one-way deserialization only), add a comment explaining why serialization is excluded.

---

Code Quality Observations

✅ Good Practices Observed:

• Consistent use of #[serde(skip_serializing_if = "Option::is_none")] for cleaner JSON output
• Proper From trait implementations for struct conversions
• Default implementations for clean initialization
• Good use of struct flattening with #[serde(flatten)]
• Tests updated consistently across all test files

✅ No Critical Issues Found:

• No SQL injection vulnerabilities
• No use of SELECT * in worker queries (adheres to project guidelines)
• Proper error handling maintained
• No blocking operations in async code

---

Performance & Security

Performance: ✅ Neutral to positive

• Struct grouping may improve cache locality
• No additional allocations introduced
• Default values avoid unnecessary Option wrapping

Security: ✅ No new vulnerabilities

• No changes to authentication/authorization
• Proper parameter handling maintained
• No unsafe code introduced

---

Testing Coverage

The PR updates test files appropriately:

• backend/tests/python_jobs.rs: All 6 test cases updated with new struct initialization
• backend/tests/job_payload.rs: Payload construction updated
• backend/tests/common/mod.rs: Helper functions updated

Recommendation: Consider adding specific tests for:

1. Deserialization backwards compatibility (old JSON format → new structs)
2. ConcurrencySettings ↔ ConcurrencySettingsWithCustom conversion round-trips
3. DebouncingSettings::is_default() edge cases

---

Backwards Compatibility

✅ Well-Handled:

• Serde aliases ensure old API clients work (custom_debounce_key → debounce_key)
• Default implementations prevent breaking changes
• Database schema unchanged (existing columns still map correctly)

⚠️ Monitor:

• Ensure CLI and frontend are compatible with the new struct format
• Verify API documentation is updated if field names changed

---

Documentation

Missing Documentation:

1. No CHANGELOG entry explaining the refactoring
2. API breaking changes (if any) not documented
3. Migration guide for external API consumers

Recommendation: Add a note in CHANGELOG or release notes mentioning:

• Settings now grouped into structs
• Old field names still supported via aliases
• No action required from users

---

Testing Instructions

To test these changes through the app interface, a tester should:

For Concurrency Settings:
Navigate to any script or flow editor, access the "Settings" or "Advanced" tab, and configure concurrency controls (concurrency key, concurrent limit, time window). Save and run the script/flow multiple times to verify concurrency limiting works correctly. Check that existing scripts with concurrency settings continue to work without re-saving.

For Debouncing Settings:
Open a flow editor, navigate to flow-level settings or module-level settings, configure debouncing options (debounce key, delay, max debouncing time, arguments to accumulate). Trigger the flow rapidly multiple times and verify that jobs are properly debounced according to the settings. Test that flows created before this change still respect their debouncing configuration.

For Backwards Compatibility:
Use the API directly (via curl or Postman) to create/update scripts and flows using the old field names (custom_concurrency_key, custom_debounce_key) and verify they still work. Check that the Windmill CLI can sync scripts/flows with these settings without errors.

---

Final Recommendation

LGTM ✅ - This is a solid refactoring that improves code maintainability without introducing breaking changes. The identified issues are minor and can be addressed in follow-up commits or ignored if the team considers them acceptable.

Suggested Follow-ups:

1. Complete the TODO documentation comments
2. Add backwards compatibility tests
3. Remove or document the unused AWS import in tests
4. Update API documentation if field names are exposed to users

---

Inline comments added for specific code locations that need attention.

[pyranota]

/eeref

[windmill-internal-app]

Starting ee ref update...

View workflow run

[windmill-internal-app]

Successfully updated ee-repo-ref.txt

[pyranota]

/eeref

[windmill-internal-app]

Starting ee ref update...

View workflow run

[windmill-internal-app]

Successfully updated ee-repo-ref.txt