The 900-Line Mop

PR #21911 adds a cleanup handler. One cleanup handler. It landed at +902/-95 across 5 files, took 12 commits to stabilize, and the PR description includes a section titled “Boundary contract preserved” that devotes seven bullet points to explaining what the code doesn’t do. When your documentation of non-behavior is longer than most people’s documentation of behavior, you’ve entered a special tier of over-specification.

The handler is called CancellingTimeoutTeardownHandler. Forty characters. Its job is to kill infrastructure after a flow run’s cancellation timeout expires. You could call it CancelTimeoutKiller and lose nothing — but that would require the kind of restraint that doesn’t produce 900-line PRs. The class docstring opens with “Idempotent infrastructure teardown for cancelling_timeout_teardown.v1” — repeating the class name back to you in slightly different words, as though the reader made it to line 28 of _cleanup_handlers.py without any idea what file they were in.

The real gem lives at the bottom of the file:

1@lru_cache(maxsize=None)
2def _class_implements_kill_infrastructure(cls: type) -> bool:
3    return sum("kill_infrastructure" in vars(c) for c in cls.__mro__) > 1

This decides whether a worker subclass actually overrides kill_infrastructure. The solution: walk the entire MRO, count how many classes define the name, and check if the count exceeds one (because the base class defines a stub). Not any(). Not an abstract method. Not a class-level flag. A sum() over string-matched vars() dictionaries, cached forever with lru_cache(maxsize=None) as though MRO traversal were the performance bottleneck standing between Prefect and production readiness. Reviewer zzstoatzz saw this and commented, in full: “waow”. Hard to argue.

Then there’s getattr on typed Pydantic models:

1flow_run_id = getattr(message.target, "flow_run_id", None)

message.target is a CancellingTimeoutCleanupMessagePayload — a Pydantic model with flow_run_id declared right there as a typed field. The PR calls this “defense in depth.” Readers of this blog may recall that the last roast called out the exact same pattern — getattr(flow, "launcher", None) — and noted that the team’s own standards say “do not use getattr; if you need it, you don’t understand the types.” Two roasts later, same sin. At this point it’s not a habit, it’s a signature.

The commit history follows the familiar desertaxle arc. It starts with “Add cancelling_timeout_teardown.v1 cleanup handler,” then immediately enters the refinement spiral: “Hoist cleanup-handler imports to module scope.” “Consolidate cleanup-handler registry construction.” “Simplify handler and tests.” “Promote BaseWorker._get_configuration to public.” “Move configuration assembly off BaseWorker.” “Update stale docstring reference to register_default_cleanup_handlers.” That last one exists because a function was renamed between earlier commits of this same PR and the docstring didn’t get the memo. Twelve commits to land a single handler. By the end, the PR has also refactored _get_configuration into a classmethod called resolve_for_flow_run and relocated it from BaseWorker to BaseJobConfiguration — scope creep so smooth you almost don’t notice it snuck an architectural migration into a cleanup handler ticket.

The handler logic itself, beneath all the ceremony, is solid. Exception mapping is specific and intentional — InfrastructureNotFound acks as idempotent success, NotImplementedError releases as unsupported_worker_type, ObjectNotFound becomes configuration_context_unavailable. The 554 lines of test coverage exercise every branch, including edge cases like empty-string PIDs and deleted flow runs. It’s 4:1 test-to-code, which would be admirable if it didn’t also mean desertaxle wrote 554 lines of tests to prove that 142 lines of cleanup code correctly take out the trash.

Final Verdict: The Overengineered Janitor — needs 900 lines, 12 commits, and a cached MRO walk to take out the trash