Closes #22872: Validate custom script Meta values before enqueueing

[jnovinger]

Closes: #22872

A custom script could declare an invalid job_timeout or notifications_default in its Meta class. Both values reached the job unvalidated: a bad notifications_default failed Job.full_clean() and a bad job_timeout failed in RQ, each raising an unhandled exception that surfaced as an HTTP 500.

Validation now happens once, at the ScriptJob.enqueue choke point, through a new BaseScript.validate_meta() classmethod. Every way of running a script funnels through ScriptJob.enqueue (interactive runs, the REST API, the runscript command, event-rule actions, and recurring reschedules), so a misconfigured script is caught before any job is created and each caller reports the error in its own idiom:

• UI: the run form re-renders with an error message
• REST API: a 400 response
• runscript command: a CommandError instead of a traceback
• Event-rule action: logged and skipped, so a misconfigured script cannot abort the object change that triggered it
• Recurring reschedule: logged against the completed job without altering its outcome (the run that just finished keeps its status and does not fire a duplicate notification)

Unset values keep their existing valid defaults, so scripts that run today are unaffected. Non-positive job_timeout values are also rejected: they parse cleanly but are nonsensical as a timeout.

[jnovinger]

@claude /review Only report on issues of potential concern; we don't need commentary on anything that doesn't require further consideration.

[github-actions]

Claude finished @jnovinger's task in 6m 57s —— View job

---

Review: #23068 — Validate custom script Meta values before enqueueing

• Gather PR context and diff
• Review validate_meta() implementation
• Review call sites (UI, API, runscript, events, reschedule)
• Review test coverage
• Post review

The choke-point approach is sound — ScriptJob.enqueue really is the single funnel, and each caller's error idiom is appropriate. Findings below, roughly by severity.

---

1. Validating Meta instead of the values actually enqueued causes a behavior regression

validate_meta() reads cls.notifications_default / cls.job_timeout off the class, but the value that actually reaches Job comes from the caller's kwargs:

• API: ScriptInputSerializer.notifications — Meta.notifications_default is only used as the field default (netbox/extras/api/serializers_/scripts.py). If the client sends an explicit notifications, the invalid default is never used.
• UI: _notifications is a ChoiceField with valid choices; as_form() only sets .initial (netbox/extras/scripts.py:573). An invalid initial doesn't match any <option>, so the browser submits the first valid choice and ScriptForm.clean()'s initial-fallback never fires.

So a script with notifications_default = 'on_error' runs fine today through both the UI dropdown and an explicit-API request, and after this PR it hard-fails with a form error / HTTP 400. Only the "quick run" path (no _notifications submitted) and the serializer-default path are actually broken today — which is exactly what the two new tests exercise.

The inverse gap exists too: a caller-supplied job_timeout that isn't from Meta still bypasses validation and blows up in the transaction.on_commit callback in Job.enqueue.

Validating the effective values would close both:

# netbox/extras/jobs.py
script_class.validate_meta(
    job_timeout=kwargs.get('job_timeout', script_class.job_timeout),
    notifications=kwargs.get('notifications'),  # None → fall back to notifications_default
)

If rejecting misconfigured scripts unconditionally is the intended (stricter) behavior, that's defensible — but it's a behavior change beyond the 500 fix and worth being explicit about in the release note. Fix this →

2. ScriptJob.enqueue narrows the base signature

netbox/extras/jobs.py:31 — def enqueue(cls, instance, *args, **kwargs) makes instance a required positional, where JobRunner.enqueue is (cls, *args, **kwargs). JobRunner.enqueue_once() calls:

return cls.enqueue(instance=instance, schedule_at=schedule_at, interval=interval, *args, **kwargs)

With any positional *args that's TypeError: got multiple values for argument 'instance'. Not reachable for ScriptJob today, but it's a latent trap for plugin subclasses. Keyword-only preserves the contract:

def enqueue(cls, *args, instance=None, **kwargs):
    ...
    return super().enqueue(*args, instance=instance, **kwargs)

3. Event-rule failures leave no trace outside the log

netbox/extras/events.py:271 — skipping the enqueue means no Job row is created, so nothing appears in the Jobs list or on the script's page. An operator whose event rule silently stops firing has only netbox.events_processor logs to go on. Creating an ERRORED Job (how other script failures surface) would be more discoverable. Deliberate trade-off either way, but worth a maintainer call.

4. runscript CommandError branch is untested

netbox/extras/management/commands/runscript.py:110 adds an except ValidationError → CommandError, but netbox/extras/tests/test_management_commands.py isn't touched. Every other caller got a test; this one is the odd one out.

5. Release note entry

AGENTS.md: "Do not include an entry in the release notes for the PR unless explicitly instructed to do so." This PR adds an entry and opens a new ## v4.6.10 section in docs/release-notes/version-4.6.md.

Minor

• netbox/extras/scripts.py:423 — AssertionError in the except tuple appears to be dead: rq 2.11's parse_timeout has no assert, only int() (TypeError) and an explicit TimeoutFormatError. The comment ("its internal int()/assert") reads as inaccurate. Also, job_timeout = True passes validation (bool is numbers.Integral, True > 0).
• netbox/extras/api/views.py:424 — raise ValidationError(e.messages) renders as a bare JSON array (["Invalid job_timeout…"]) rather than NetBox's usual object-shaped error body. {'detail': e.messages} or {api_settings.NON_FIELD_ERRORS_KEY: e.messages} would be more consistent; raise … from e preserves the chain.
• netbox/netbox/jobs.py:8 — the message built with gettext_lazy only ever lands in logger.error() and the job log; the surrounding log statements in this module aren't translated.

---

Correctness of the core fix looks right otherwise: validation runs before any Job row is created (previously the job_timeout failure happened inside the on_commit callback, leaving an orphaned PENDING job), parse_timeout/TimeoutFormatError imports match rq 2.11, and the reschedule handler correctly avoids re-running terminate() — Job.log() only mutates log_entries in memory, so the added job.save() is required.

I did not run the test suite (no DB/Redis in this environment).
• branch 22872-invalid-script-meta-500

[jnovinger]

Validating Meta instead of the values actually enqueued causes a behavior regression

Fixed in 4330376. validate_meta() now takes the effective job_timeout and notifications and only falls back to the Meta default when the caller omits them, so an explicit value is validated and a valid one is no longer rejected over an unused default.

ScriptJob.enqueue narrows the base signature

Fixed in 4330376: instance is keyword-only now, so the override keeps JobRunner.enqueue's signature and can't collide with a positional in enqueue_once().

Event-rule failures leave no trace outside the log

Keeping log-and-continue here to match the existing pipeline: flush_events logs and continues, and the webhook branch doesn't validate at all. An ERRORED Job would be more discoverable, but my view is that belongs in a follow-up applied uniformly across event actions rather than the script branch alone.

runscript CommandError branch is untested

Added a test in 4330376.

Release note entry

Removed in 4330376. Whoops, let Claude get that one by me.

AssertionError in the except tuple appears to be dead

parse_timeout in rq 2.11 still runs assert isinstance(timeout, str) after the int() ValueError path, so the catch is defensive rather than dead. Keeping it.

raise ValidationError(e.messages) renders as a bare JSON array

Fixed in 4330376: raised under the detail key and chained with from e.

job_timeout = True passes validation

True is numbers.Integral and parses to 1, which is harmless as a timeout, so I'm leaving it rather than special-casing bools.

[pheus]

Thanks!

I found one remaining compatibility issue that I think needs to be addressed before merge: positional instance calls are still broken. I’ve left an inline comment with the details and a suggested regression test.

[pheus]

This looks like another shared-RQ-queue race rather than a rollback failure. The two successful ScriptJob.enqueue() tests execute their on-commit callbacks against the real default queue, leaving one Redis job each. Under the parallel runner, this event-rule test can then observe both jobs and fail with 2 != 0.

Could we keep those callbacks unexecuted and assert that they were registered instead? That should retain the forwarding coverage without allowing these tests to affect another worker's queue.

[jnovinger]

Could we keep those callbacks unexecuted and assert that they were registered instead? That should retain the forwarding coverage without allowing these tests to affect another worker's queue.

This (plus the test serialization of tests in that class) was the answer. Thanks!

[pheus]

Thanks for the update! This looks good to me!