You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "henry3260 (via GitHub)" <gi...@apache.org> on 2026/09/22 13:20:38 UTC
[PR] Reject invalid Go SDK task handler registrations up front [airflow]
henry3260 opened a new pull request, #73549:
URL: https://github.com/apache/airflow/pull/73549
`airflow.TaskHandler` already rejects a handler whose signature the runtime cannot call, so the mistake stops the executable as it starts rather than when the task first runs. Two registrations still slipped through and failed much later, or not visibly at all.
### An unusable dag_id or task_id
`TaskHandler("", "transform", fn)` was accepted. The empty id lands in the manifest, the runtime never matches it when the supervisor asks for the task, and the task instance is reported `removed` — so the author sees a task that silently went missing rather than the typo that caused it. The same holds for an id carrying characters Airflow rejects, or one past the 250-character limit.
Those rules already existed as private constants in the bundle packer, which warns about them after the fact. They now live in `internal/airflowid` so registration and the packer enforce one definition rather than two copies that can drift. The packer's warnings are unchanged, which its existing tests pin.
**`..` is deliberately not part of the verdict.** The server rejects it only when `[core] allow_double_dot_in_ids` is off, and a bundle cannot see the configuration of a deployment it has not reached yet — rejecting it here would refuse ids that the target Airflow accepts. The packer keeps warning about it, and a test pins the acceptance so the omission is not mistaken for an oversight.
### A variadic task function
`func(actx airflow.Context, names ...string) error` passed the signature check as if its `...` parameter were an ordinary `[]string`. It registered cleanly and then panicked inside reflect the first time the supervisor ran it: `Execute` calls the function with `Call`, which hands a variadic parameter the slice itself instead of spreading it. No argument shape can fill a `...` parameter on this path, so the registration can never become valid — `[]string` takes the same values and works.
### Not in scope
- **Registration does not close at `Serve`.** ADR 0007 says registering after `Serve` is a programming error that panics; `BundleRef` has no such guard yet. It belongs with this work but touches the serve path and its locking, so it is left for a follow-up.
- **An empty bundle is still accepted.** That is a bundle with no registrations rather than an invalid one.
- **The docs still describe the signature check alone.** `go-sdk/README.md` and the Go SDK page under `airflow-core/docs/` are being rewritten for the new authoring surface in #73528; they gain the id and variadic rules once that lands, rather than conflicting with it here.
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes — Claude Code (Opus 5)
Generated-by: Claude Code (Opus 5) following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject invalid Go SDK task handler registrations up front [airflow]
Posted by "henry3260 (via GitHub)" <gi...@apache.org>.
henry3260 commented on code in PR #73549:
URL: https://github.com/apache/airflow/pull/73549#discussion_r4079527050
##########
go-sdk/airflow/bundle.go:
##########
@@ -86,17 +87,33 @@ type taskHandlerMap struct {
mu sync.RWMutex
handlers map[string]map[string]bundle.Task
order []bundle.TaskHandlerInfo
+ closed bool
Review Comment:
One thing while we are here `Bundle` in #71188 gained `register` and a mutable `dags`, so Java can register after `serve` too, and I could not find an equivalent check there. but the two SDKs probably want the same shape?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject invalid Go SDK task handler registrations up front [airflow]
Posted by "henry3260 (via GitHub)" <gi...@apache.org>.
henry3260 commented on code in PR #73549:
URL: https://github.com/apache/airflow/pull/73549#discussion_r4079504205
##########
go-sdk/airflow/bundle.go:
##########
@@ -86,17 +87,33 @@ type taskHandlerMap struct {
mu sync.RWMutex
handlers map[string]map[string]bundle.Task
order []bundle.TaskHandlerInfo
+ closed bool
Review Comment:
> Would it make more sense to have the `closed` at the bundle instance (`BundleRef`) instead of storing at the `taskHandlerMap`? Since the native Dag will encounter the same situation.
Good call moved to `BundleRef`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject invalid Go SDK task handler registrations up front [airflow]
Posted by "henry3260 (via GitHub)" <gi...@apache.org>.
henry3260 merged PR #73549:
URL: https://github.com/apache/airflow/pull/73549
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject invalid Go SDK task handler registrations up front [airflow]
Posted by "henry3260 (via GitHub)" <gi...@apache.org>.
henry3260 commented on PR #73549:
URL: https://github.com/apache/airflow/pull/73549#issuecomment-5779527842
> https://github.com/orgs/apache/projects/499/views/7?pane=issue&itemId=248986989
>
> > * Registering after `Serve` has been called panics - registration closes when serving starts.
> > * A duplicate `(dagId, taskId)` panics.
> > * A struct carrying `arg:` tags mixed with other data parameters is rejected (an untagged struct has no such guard and is decoded positionally as one value).
> > * Signature errors - non-function, wrong return shape, missing leading `airflow.Context` - panic with the offending function named.
>
> It seems the scope of current PR is not align with the issue scope. How about checking whether all the above issue scope are covered instead of having current refactor.
>
> Additionally, the task id validation should be in compile time instead of at the task runtime. IIUC, the current refactor will introduce side-effect (might raise validation error) during the runtime.
Thanks I had misunderstood earlier.
Implemented the first point instead: registration now closes in `Serve`, and a later `Register` panics. It was the only one of the four `main` did not already satisfy.
I kept the variadic check as a signature error of the same class: `Execute`calls the handler with `Call`, which cannot spread into a `...` parameter, so it registers cleanly and panics inside reflect on first run. It sits in `validateFn` beside the existing checks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reject invalid Go SDK task handler registrations up front [airflow]
Posted by "jason810496 (via GitHub)" <gi...@apache.org>.
jason810496 commented on code in PR #73549:
URL: https://github.com/apache/airflow/pull/73549#discussion_r4074188284
##########
go-sdk/airflow/bundle.go:
##########
@@ -86,17 +87,33 @@ type taskHandlerMap struct {
mu sync.RWMutex
handlers map[string]map[string]bundle.Task
order []bundle.TaskHandlerInfo
+ closed bool
Review Comment:
Would it make more sense to have the `closed` at the bundle instance (`BundleRef`) instead of storing at the `taskHandlerMap`? Since the native Dag will encounter the same situation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org