UN-4136 [FIX] Stop the file_processing worker booting the executor stack on each child's first task - #2293
UN-4136 [FIX] Stop the file_processing worker booting the executor stack on each child's first task#2293ritwik-g wants to merge 12 commits into
Conversation
Each pg_queue_consumer child paid ~9s of lazy initialisation on its first task. The supervisor preforks 20 children per pod, so that is ~170 CPU-seconds per pod spent during serving rather than at startup, which spikes fleet CPU at load onset and inflates CAST AI's WOOP p99 to 792m against a 200m request -- putting the HPA signal's ceiling below its own target so the fleet collapses to minReplicas under backlog. structure_tool_task.py imports two string constants and a StreamMixin shim from the executor package. None needs the executor registry, but every import under `executor` booted the whole thing: executor/__init__.py imported .worker, which imported executor.executors, which imported LegacyExecutor and ran entry-point discovery. Measured at 9.98s cold. Resolve celery_app lazily (PEP 562) and replace the executors package's import-side-effect registration with an explicit, idempotent register_all(). The two entrypoints that need a populated registry -- executor/worker.py and executor/tasks.py -- call it directly; both already carried an explicit `import executor.executors # noqa: F401` for exactly this purpose, so this makes an existing intent explicit rather than changing it. structure_tool_task.py is untouched; all three of its imports become cheap. Deferred first-task cost goes 2.334s -> 0.004s locally, with legacy_executor no longer imported into file_processing at all. The new test pins the import graph rather than a duration -- a timing assertion would be flaky on CI, while what actually regresses is an eager import creeping back into either package init. Both cases were confirmed failing on origin/main. UN-4136 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Critical: restore the celery.app.trace suppression. executor/worker.py's setLevel(WARNING) mutes the trace logger, which prints the full task result — for execute_extraction, extracted customer document text — at INFO. That module is on no deployed path: workers/worker.py exec-loads executor/tasks.py by path for both executor roles and nothing runs `celery -A executor`. It ran only because executor/__init__.py eagerly imported .worker, so making that lazy silently re-enabled result logging on every extraction. Moved the suppression to executor/tasks.py, which keeps it without re-importing the expensive module. register_all(): latch before discovering rather than after. ep.load() executes third-party code, and a plugin whose import graph reaches back into the function would restart the entry point loop, nesting once per level (measured: 50 nested loads where 1 was intended). The import-side-effect version got that safety free from sys.modules. A discovery that raises now resets the latch instead of pinning a permanently-empty list. Correct the docstrings rather than the behaviour where the claim was the defect. register_all() cannot repopulate a registry something else emptied — the second import is a sys.modules hit and the decorator does not re-run. Making it re-register explicitly broke four test files that do unguarded ExecutorRegistry.register(LegacyExecutor) after a clear, so the guarantee is now stated as per-process instead. Also: executor/__init__.py no longer contradicts its sibling in the present tense; worker.py's register_all() call is gone (it could never do work — import executor.tasks above it already registered, so the comment claiming otherwise would have led a maintainer to delete the call that matters); plugins/loader.py and test_plugin_loader.py's own enumeration no longer describe the removed import side effect. Tests: all three new cases were defective. One was vacuous — removing the latch entirely left the suite green, because with no cloud plugins installed the first call also returns []. Two passed only on suite ordering: an unrelated module writes ExecutorRegistry._registry["legacy"] directly and never cleans up, and CI shards with xdist's default per-test scheduler, so their result depended on shard assignment. Rewritten to assert registration in a fresh interpreter, to assert idempotency on a spy's call count rather than a return value that is [] either way, and to snapshot/restore the process-global registry. Added coverage for re-entrancy, for the non-latching failure path, and for the trace suppression. All five guards are mutation-verified. UN-4136 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-review round 1 —
|
| Lens | Finding | Fix |
|---|---|---|
| 14/6 | Re-entrancy. Latch set after discovery, so a plugin reaching back into register_all() during ep.load() restarts the entry-point loop. Measured 50 nested loads where 1 was intended; the import-side-effect version got this free from sys.modules. |
Latch before discovering |
| 8 | A discovery that raises latched a permanently-empty list | except BaseException resets it; retry works |
| 7/16 | register_all()'s docstring promised a postcondition it cannot meet — it cannot repopulate a cleared registry (second import is a sys.modules hit) |
Docstring narrowed to the per-process guarantee — see note below |
| 16 | executor/__init__.py stated, present tense, the cost model this PR removes, contradicting its sibling docstring |
Past-tensed |
| 7 | LegacyExecutor lost its static type — no TYPE_CHECKING counterpart, unlike celery_app |
Added |
Note on the postcondition finding. My first fix made register_all() repopulate a cleared registry. That broke test_line_item_extraction with ValueError: Executor name 'legacy' is already registered — four test files do unguarded ExecutorRegistry.register(LegacyExecutor) after a clear. On origin/main that pairing passes, so it was a real regression. Reverted: the defect was the claim, not the mechanism, and fixing the docstring doesn't touch four unrelated files.
⚪ Low
_MUST_NOT_LOAD's rationale was backwards (legacy_executor imports the x2text adapter stack at module scope — it is the expensive module); a cloud-repo path cited as if local; unreproducible pod-name/prefork-count measurements trimmed to one anchored figure; lens 17 — plugins/loader.py and test_plugin_loader.py's own "Verifies" enumeration both still described the removed import side effect.
Examined and clean
Lens 17 branch-currency: base has not advanced (git log $MB..origin/main empty, exit 0). Lens 7 consumer search across all five workspace repos: zero consumers of the changed surface beyond the files updated here; all 8 unstract.executor.executors entry points import submodules only, never the bare package, including function-local imports. Boundary sweep: 2 sinks traced (both AttributeError messages, internal identifiers only), clean. Lens 15: all pinned pre-commit hooks pass and modified nothing. Lens 6 across fork(): no divergence — the supervisor imports in the child deliberately. Lens 11: no readiness window — registration precedes the @worker_task decorator. Lens 19: the change only shortens the heartbeat freeze, moving oldest_age() away from the staleness threshold. Lens 14: no build/Docker/CI dependency on the old side effect.
Coverage gaps, stated rather than glossed
- Lens 3 not covered — the dispatched agent returned a completion signal but no report body. Re-requested; not recorded as clean.
- Cloud entry-point executors are not installed locally, so registration is exercised for
legacyonly. Their independence from the removed side effect is verified by source sweep, not by running them. - Step 2a security deep-dive not triggered: no endpoint, parser, authn/authz path, untrusted-input sink or credential in the diff.
cost: agents=8 wall=~42:00 lenses-examined=18 of 19 files=6 lines=264
Round 2 is owed on the delta these fixes create — git diff bf3cab1b0 98965e12e, 7 files / +257−96, entirely unreviewed.
_reset_discovery_for_tests()'s docstring still claimed register_all() re-registers the bundled executor when the registry has lost it. That was true of a fix I reverted — making register_all() re-register broke four test files that do unguarded ExecutorRegistry.register(LegacyExecutor) after a clear — and the stale claim then contradicted register_all()'s own docstring. It sat on the helper a test author reads before writing `clear(); reset(); register_all()` and asserting on the registry, which returns cleanly and leaves it empty. register_all() now returns a copy. It was handing out the latched module list, so a caller appending to the result corrupted what every later caller saw. Subprocess test snippets raise SystemExit instead of asserting. The child inherits the parent environment, so under PYTHONOPTIMIZE every assert is stripped: the snippet would print OK, exit 0, and the guard would pass having checked nothing. Verified by mutating the trace suppression under PYTHONOPTIMIZE=1 and confirming the test still fails. Test coverage for three guards that had none: the LegacyExecutor __getattr__ shim (no caller in this repo or the cloud plugins uses that spelling, so a typo in the name comparison would break every cloud plugin while OSS CI stayed green), the breadth of the BaseException catch (the failure test drove only RuntimeError, so narrowing the handler would have kept the suite green while reopening the latched-empty window for the KeyboardInterrupt case it exists for), and the copy semantics above. Documented, not fixed: discover_executors() catches per-entry-point failures and logs a warning, so register_all() latches [] and returns it whether no cloud plugins are installed or every one of them failed to import. A broken plugin wheel therefore boots clean and surfaces per-request as "No executor registered with name 'table'". The Returns section now states both meanings rather than only the benign one. Making the aggregate loud is a behaviour change to the loader and out of scope for a performance PR. UN-4136 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every finding this round was introduced by round 1's fixes. Drop the reset-on-failure rather than patch it. It was added so "a caller that survives the failure retries instead of silently getting a permanently empty list", but there is no such caller — the single call site is at executor/tasks.py module scope, so a failure kills the boot and the process restarts with clean module state. Worse, the retry it enabled is harmful: a plugin whose @ExecutorRegistry.register fires before it raises is evicted from sys.modules but left in the registry, so re-importing it raises a duplicate-name ValueError that discover_executors swallows into a warning, leaving that executor silently absent for the life of the process — the exact outcome the reset claimed to prevent. Verified. Dropping it also closes the window where the latch was armed one statement before the try, and removes the BaseException breadth question along with the test that pinned it. Stop the test fixture corrupting the registry it was added to protect. clear() + update(saved) deletes a registration the test itself caused: when this class is what first imports legacy_executor, the decorator fires during the test so `legacy` is absent from the snapshot, and it cannot be restored because the re-import is a sys.modules hit. Every later test in that process then saw an empty registry; nothing failed only because five other modules re-register defensively. Now an additive restore. Restore test_register_all_tolerates_legacy_executor_already_imported, deleted in round 1. Its scenario is live in production — a cloud plugin imports LegacyExecutor at module scope and is loaded by ep.load() inside discovery — and the subprocess replacement covers the opposite ordering, a fresh process with no prior import. Deleting it weakened coverage for a real path. Convert the remaining subprocess snippet to raise SystemExit. Round 1 wrote that rule into one test file and violated it in the other in the same commit; under PYTHONOPTIMIZE the asserts are stripped and the guard passed having checked nothing. Confirmed by mutation under PYTHONOPTIMIZE=1. Docstrings: the module docstring still described a register_all() call in executor/worker.py that the same commit deleted, and tasks.py still called that second call "harmless" — both now say executor/tasks.py is the only caller and that worker.py's import of it is load-bearing. The Returns block promised the names "on every call" when the re-entrant call it advertises returns the empty placeholder, and counted two causes for an empty list when there are three. The claim that tests clearing the registry re-register explicitly was false for four of the five that clear it. The xdist claim is narrowed to the repo's test rig, which is what is checkable — there is no .github/workflows here. UN-4136 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-review round 2 —
|
The lazy import moved WHEN legacy_executor first loads, and two test files depended on the old timing. On main, importing executor.executors registered `legacy` at package-import time — before any fixture cleared the registry — so an unguarded ExecutorRegistry.register(LegacyExecutor) in the test always followed a cache hit. With the import lazy, that first import now happens inside the test, after the clear, and the explicit registration raises a duplicate-name ValueError. tests/test_legacy_executor_index.py and tests/test_legacy_executor_extract.py fail alone on this branch and pass alone on main; they survive the full alphabetical suite only because an earlier module imports the class first, which is why this looked pre-existing. Guarded both, matching the pattern eight sibling modules already use. Restore the except BaseException reset removed in round 2. That removal rested on a false premise: the handler never retried, it re-raised unchanged, and its only effect was to stop a LATER call reporting "no cloud executors" as though discovery had succeeded. It also preserves pre-PR behaviour — discovery ran as a package import side effect, and a module that raises mid-import is evicted from sys.modules, so the next import re-ran it. The duplicate-registration hazard cited as the reason to drop it applies to re-running discovery at all, so it predates this function and was never traded away by keeping the reset. Restore the clearing teardown, also removed in round 2. It was made additive to stop it deleting a registration the class itself caused, but nothing in the suite depends on import-side-effect registration of `legacy` — every consumer registers explicitly or assigns directly — so the snapshot restore was correct isolation and the additive version leaked `legacy` into every later test in the worker. Worse, the one test that ran unpatched discovery would, where the cloud wheels are installed, have leaked all eight cloud executors into names the sibling modules register themselves. That test is rewritten: it arranges its own precondition instead of inheriting one, patches discovery so it stops doing real entry-point loading, and asserts only the property it is named for. As written it was order-dependent — it failed deterministically after any module that clears the registry — and its registry assertion was satisfied by its own import line rather than by register_all(). Docstrings: "the only caller" was false (the suite calls it in-process) and the handler-removal argument was reasoned from it; restored the "production" qualifier and the note that executor/worker.py's app is test-only. The stated failure symptom for a broken plugin wheel held only when the plugin raises before its decorator. Cited the rig path repo-root-relative. All six guards mutation-verified. UN-4136 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-review round 3 —
|
| Effect | Disposition | |
|---|---|---|
| L17 | WorkerLogger.setup(EXECUTOR) |
Not lost — worker.py:52 configures for the selected type before exec-loading tasks.py, and configure is _configured-latched. Verified: logging state byte-identical to main after the real bootstrap |
| L18 | build_celery_app(EXECUTOR) |
The cost being removed |
| L23 | celery.app.trace suppression |
Round 1's Critical — nothing else replicated it |
| L64 | register_health_check |
Harmless — its only reader is reachable solely from the Celery build path; the PG role runs its own LivenessServer |
Correction to round 1's and round 2's reports
I described the Critical as the class of thing the ticket's three variants would all have hit. That is wrong: variant 1 never touches executor/__init__.py, and variants 2 and 3 keep the import. The hazard belongs to this approach, and to any future trimming of that import.
cost: agents=5 lenses-examined=18 of 19 files=3 lines=134
Round 4 is owed on fdad45bad..6156593a8 — 4 files, +120/−55.
Guard the two remaining duplicate-registration sites. test_legacy_executor_ scaffold.py is a proven regression from this branch, not pre-existing as an earlier check concluded: run alongside the index and extract modules it passes on main (52 passed) and fails here. My earlier check ran that file ALONE, where it is broken on main too for its own reason, which hid the difference. Mutation- verified: removing the guard reproduces the failure in that ordering. test_plugin_migration_regression.py has the identical shape; its guard is defensive consistency and is NOT pinned by a test — that file's own earlier tests import legacy_executor before the helper runs, so no ordering within it exercises the guard. Recorded rather than claimed as covered. "This does not retry" was true of the handler and false of the mechanism: un-arming the latch is exactly what lets the NEXT call re-run discovery, and the comment then justified itself by saying the next import re-ran it. Both cannot hold. It now says plainly that it re-arms a retry for a later caller, that the re-run is not free when a plugin registered before it raised — that plugin ends up live in the registry but absent from the returned list — and that no production caller survives the re-raise to make that call. The Returns block now says a non-empty list can under-report the registry after a failed-then-re-run discovery. The _cloud_executors sentinel comment said None means "discovery has not run"; since the handler came back it also means "ran and raised". The empty-list placeholder moved inside the try, so an async signal between the assignment and the try cannot leave it armed. The register_all docstring no longer offers test_legacy_executor_scaffold.py as "the pattern to copy" — that file carried the unguarded helper this commit fixes; it states the guard rule directly instead. TestExecutorsInit's docstring claimed the fixture leaves the process as it found it. sys.modules is not restorable and this class can be the first importer of legacy_executor, which is the state the sibling guards exist to survive. test_failed_discovery_un_arms_the_latch is parametrised rather than looped: a for loop stops at the first failing arm, and the KeyboardInterrupt arm is the one the docstring argues is the point. Its docstring no longer says only BaseException reaches the handler — an entry_points() failure is an ordinary Exception and reaches it too, which is what the RuntimeError arm covers. UN-4136 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-review round 4 —
|
| Mutant | Result |
|---|---|
| reset handler removed | KILLED |
BaseException → Exception |
KILLED |
[] placeholder removed |
KILLED |
| scaffold guard removed | KILLED (with scaffold-first ordering) |
| migration-regression guard removed | SURVIVED |
The last one is honest rather than fixed: that file's own earlier tests import
legacy_executor before the helper runs, so no ordering within it exercises
the guard. It is defensive consistency against a latent instance of a class
proven live elsewhere, and is recorded as unpinned rather than counted as
covered.
Full suite: 1335 passed, 12 pre-existing failures, zero regressions.
Lens 15 pinned hooks pass. Lens 17 clean. Step 2z: no sink change. Lens 4 / 2a:
no trigger.
Coverage — two lenses NOT covered this round
- Lenses 1, 2, 3, 9, 10, 15 (correctness agent) —
Not covered — agent terminated on a session rate limit mid-run. - Lens 13 (test-analysis agent) —
Not covered — agent terminated on a session rate limit mid-run.
Lens 3 was covered by the dedicated silent-failure agent, which produced two of
this round's findings. Lens 13 was partially covered by the cold read. Neither
substitutes for the dispatched specialist, so both are recorded as gaps rather
than inferred clean.
Round 5 runs a full fan-out over 6156593a8..0541d0512, which subsumes the
uncovered lenses and reviews this round's fixes together. Re-dispatching the
dead agents against the old delta would review a scope that no longer exists.
One check that came back clean for the first time
The cold read was asked to verify a count-of-its-own-enumeration claim against
its constraining sentence. It enumerated all seven methods in TestExecutorsInit
and concluded: "Count = 1. The claim holds. I could not break it."
cost: agents=5 (2 died) lenses-examined=16 of 19 files=4 lines=175
…g prose tests/test_line_item_extraction.py was the last site with the hazardous ordering — autouse fixture clears the registry, then the test imports legacy_executor and registers it unguarded. When that import is the first in the process the decorator fires after the clear and the explicit register raises. Fails alone on this branch, passes alone on main. Routed through the module's existing guarded helper. Mutation-verified. An enumeration of all 22 ExecutorRegistry.register sites under workers/tests/ says this was the last one of its shape: 13 already guard, 2 import before they clear and are safe by ordering. Delete the historical narrative from the guard comment in all four files. It was written for index/extract, where it is true, and copied into scaffold and migration_regression, where it is false: scaffold fails alone at the merge base, so it was never "unguarded safely", and in both of those the first import of legacy_executor was already inside a test before UN-4136, so UN-4136 moved nothing. Only the rule survives, which is true everywhere. The prose in register_all() has now been wrong in four consecutive rounds, each time by being true of the case it names and false of one it never mentions, so this trims rather than rewrites: - the guard rule stated one implementation as mandatory and gave a reason that inverted the actual cause (the hazard is the import has NOT happened yet); it now states the invariant, and notes clearing immediately before is equally valid, which is what test_summarize_operation.py does; - the under-report note was scoped to a non-empty list, but with a single installed plugin the re-run returns [] — folded into the empty-list enumeration as "at any length, including zero"; - the handler comment claimed every already-registered plugin is evicted from sys.modules. Only the module that was mid-import when the exception escaped is. Narrowed, and the surrounding argument cut to what is checkable. Also reverts formatter churn this branch introduced in two test files: a `ruff format` run reflowed an aligned comment block unrelated to the guard. UN-4136 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ollow-up TestExecutorsInit's docstring said the sibling guards "assume" the sys.modules entry for legacy_executor is left behind. They assume the opposite: the guard is INERT when the entry is present (the import is a cache hit, the decorator does not fire, the cleared registry has no `legacy`, the register runs) and load-bearing when it is absent. The previous commit fixed this same inversion in register_all()'s docstring and missed this sentence. Record in code that the guard idiom is hand-copied across 23 sites in 18 files, in three spellings, and that folding it into a shared test helper is the follow-up rather than something this PR does. Counted: 23 guard sites, 7 direct _registry["legacy"] assignments, 1 clear-then-register. 18 of the 23 predate this branch. Detail on UN-4136. Correcting the record on the previous commit: it stated the test_plugin_migration_regression guard is "NOT pinned by a test". That is wrong. No whole-FILE run pins it, but three class subsets fail without it, and the rig runs xdist with the default per-test `load` scheduler, so the unit of ordering in CI is the node. Measured with the guard removed: whole-file: 31 passed <- what the mutation gate was measuring node-level: 3 classes fail <- the guard is load-bearing That wrong granularity is also why the test_line_item_extraction site survived four rounds of module-ordering A/B: a defect that only appears when a node runs without its file-mates is invisible at module granularity. Mutation checks on the remaining guards were re-run at node level. UN-4136 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-review round 5 —
|
| node | main | branch |
|---|---|---|
scaffold::TestRegistration |
1 failed | 1 passed |
migration::TestMultiExecutorCoexistence |
1 failed, 3 passed | 4 passed |
migration::TestGracefulDegradation |
1 failed, 2 passed | 3 passed |
line_item::TestLineItemEndToEnd |
1 passed | 1 passed |
The branch is strictly better than main at node granularity: three classes that
fail on origin/main pass here, none regress.
🟡 Recorded, not fixed — the rule is hand-copied ~30 times
The guard idiom appears 23 times across 18 files, plus 7 sites that assign
ExecutorRegistry._registry["legacy"] directly and 1 that clears immediately
before registering. Three spellings of one rule; 18 of the 23 predate this
branch. Nothing enforces any of them — a new test module gets it right only by
remembering, and this PR's own five rounds are the evidence that it is
forgettable.
The fix is a shared register_legacy() helper (or an idempotent
ExecutorRegistry.register_once) so omission becomes impossible. That is a
test-infrastructure refactor across 23 files with its own blast radius, so it is
recorded rather than done: noted in register_all()'s docstring and detailed
on UN-4136.
🟡 Prose, cut rather than rewritten
TestExecutorsInit's docstring said the sibling guards "assume" the
sys.modules entry is left. They assume the opposite — the guard is inert
when the entry is present and load-bearing when it is absent. a0d4c2e31 fixed
this same inversion in register_all()'s docstring and missed this sentence;
3dc477cea closes it.
The round-5 commit also deleted the historical narrative from the guard comment
in all four files. It was written for index/extract, where it is true, and
copied into scaffold and migration_regression, where it is false — scaffold
fails alone at the merge base, so it was never "unguarded safely", and in both
files the first import was already inside a test before UN-4136.
Net for that commit: −91 lines against +73. After four rounds of prose being
wrong the same way — true of the case it names, false of one it never mentions —
the fix was subtraction, not a fifth rewrite.
⚪ Also fixed
Formatter churn this branch introduced: a ruff format run reflowed ~30 aligned
comment lines in two test files, unrelated to the guards and undeclared. Reverted;
guards re-applied clean. An independent AST comparison confirms the reflow changed
no assertion (33→33 and 53→53, structurally identical, no assert (cond, msg)
tuple trap).
Gates
Mutation gate, re-run at node granularity: both round-5 guards killed.
Independently, an agent verified the guards do not weaken their tests — mutating
LegacyExecutor.name makes the guard-skipping subsets fail, so the skip branch
is never vacuous.
Full suite: 1335 passed, 12 pre-existing failures, zero regressions.
Node sweep: 339/339 pass. Lens 15 pinned hooks pass; lint counts match main
exactly. Lens 17 clean. Step 2z: no sink change. Lens 4 / 2a: no trigger.
Round 6 is owed on 0541d0512..3dc477cea.
Both are the same failure this PR keeps repeating — saying more than is true — so both are trimmed rather than restated. The register_all() docstring claimed the guard rule is hand-copied across "~23 sites in three spellings". The 23 is the count of the guard IDIOM, but only 16 of those are followed by ExecutorRegistry.register; the other 7 assign _registry["legacy"] directly, which cannot raise, so the rule does not reach them. The count also excluded the third spelling's only site, so it matched two of the three it claimed. Dropped: a tally of test-directory sites in a production docstring rots on the next test added, and the duplication is the fact worth recording, not the number. It also pointed the follow-up at UN-4136, which is the ticket for THIS change — cited as the problem being fixed at four other sites on the branch. A reader following it lands on the ticket this PR closes and concludes the follow-up shipped. The follow-up is recorded on that ticket as a comment and in the PR; the docstring now just says it is deliberately left out. TestExecutorsInit's docstring said the sibling guards are "inert when the entry is present and load-bearing when it is not". True of the four modules whose helper calls register(), false of the 7 direct-assign sites, where the guard is inert in both states. The surviving sentence — the guards are correct either way, so leaving the sys.modules entry is harmless — is true unconditionally. Verified while checking this: all three touched test files now differ from origin/main by their guard alone, with no formatter churn remaining. UN-4136 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… handler Round 5's trim replaced a correct enumeration with a broader one that is false. ep.load() sits inside discover_executors' per-entry-point `except Exception` (loader.py:48-56), so an ordinary exception from a plugin never escapes — it is logged and skipped there. Only a non-Exception BaseException, or a failure of entry_points() itself, reaches the handler. The risk is not the wording. A maintainer who believes anything out of ep.load() reaches this handler has no reason to keep loader.py's per-entry-point catch, and removing it turns "one broken plugin wheel is logged and skipped" into "one broken plugin wheel aborts discovery" — on the executor worker's boot path. The comment now says what that handler is for rather than enumerating what escapes. Verified both ways: read against loader.py, and by driving a fake entry point that raises RuntimeError — register_all() returns [] rather than raising. UN-4136 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…test Third writing of this sentence, third distinct falsehood. The latest claimed an ordinary exception from a plugin "never reaches here". It can: entry_points() sits OUTSIDE discover_executors' per-entry-point try (loader.py:46 vs :48), so a wheel with malformed entry-point metadata raises an ordinary Exception that escapes straight to this handler. Reproduced by building a dist-info whose entry_points.txt has a garbage line — TypeError, isinstance(Exception) True, escapes discover_executors. So the previous wording was wrong in the other direction from the one before it, and the one before that was wrong differently again. Each rewrite was a fresh surface for the same defect. Replaced with a pointer to test_failed_discovery_un_arms_the_latch, which is parametrised over both classes that can reach the handler — an ordinary Exception out of entry_points(), and a non-Exception BaseException out of ep.load() — and pins them by executing rather than by asserting in prose. Both arms pass and report independently. The correctly-scoped statement of what the per-entry-point handler absorbs already lives in the module docstring, which says "failed to import" rather than the broader "broken wheel" that made the last version false. UN-4136 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two below-floor items from the final review round. The pointer to the test that pins the BaseException choice gave a bare function name, while the file's two other test references both carry paths. A pointer's whole value is that it resolves, and a grep that comes back empty after a rename is indistinguishable from a typo. It now reads tests/test_plugin_loader.py::test_failed_discovery_un_arms_the_latch. The test docstring named KeyboardInterrupt/SystemExit as the classes escaping ep.load() while only KeyboardInterrupt had an arm. Rather than argue which reading was intended, SystemExit now has one — it is free, since the handler treats them identically. Verified it discriminates: with except BaseException narrowed to except Exception, the RuntimeError arm passes (as it must) and both the KeyboardInterrupt and SystemExit arms fail. The docstring also stops saying "both classes", which was only true while there were two arms, and now describes the partition instead: an ordinary Exception escaping entry_points(), and the BaseException-not-Exception classes escaping ep.load(). UN-4136 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-review rounds 6–8 — loop closedFinal tip: Round 8 returned no finding above the severity floor, and its delta contained Round 6 —
|
| # | claim | why it was wrong |
|---|---|---|
| 1 | "does not retry" | un-arming the latch is a deferred retry |
| 2 | "anything at all out of ep.load()" |
ep.load() is inside except Exception |
| 3 | "an ordinary exception never reaches here" | entry_points() is outside that try |
So it was deleted rather than rewritten a fourth time, and replaced with a
pointer to the test that pins the behaviour executably. The risk was never the
wording: a maintainer believing version 3 had no reason to keep loader.py's
per-entry-point catch, and removing it turns "one broken wheel is logged and
skipped" into "one broken wheel aborts the executor's boot".
Round 8 — 2cae923da..526c2e389 · nothing above the floor
The surviving claim — "every class that reaches here" — was verified
by mutation, not by reading: narrowing except BaseException to
except Exception fails the KeyboardInterrupt and SystemExit arms while
RuntimeError passes, as it must. Two Low items taken: the pointer now carries
its file path (matching the file's own convention), and SystemExit has its own
arm rather than being named in prose without one.
Why the comment kept failing — the one durable lesson
Prose asserting what reaches an exception handler cannot be checked, so each
rewrite re-derived a reachability argument from memory and got a different part of
it wrong. A parametrised test asserting the same thing fails when it stops being
true. Every round spent rewriting that paragraph was a round spent maintaining an
unverifiable claim.
Final state
| Full suite | 1336 passed, 12 pre-existing failures, zero regressions (baseline 1325) |
| Node sweep | 339/339 — every node in all 21 registry-touching modules, each in its own process |
| Mutation gate | every guard killed, at node granularity |
| Guards non-vacuous | proven by mutating LegacyExecutor.name — the guard-skipping subsets fail |
vs origin/main at node granularity |
strictly better — 3 classes that fail on main pass here, none regress |
| Pinned pre-commit | all hooks pass, nothing modified; lint counts match main exactly |
What the loop found that the change did not intend
- Critical — the lazy import silently dropped
executor/worker.py's
celery.app.tracesuppression, which would have logged ~1KB of extracted
customer text at INFO on every extraction. - Re-entrancy — a plugin calling back into
register_all()restarted the
entry-point loop (50 nested loads vs 1). - Registration timing — the lazy import moved when
@ExecutorRegistry.register
fires relative to test fixtures, breaking four test files. - A pre-existing failure fixed incidentally —
test_legacy_executor_scaffold
fails alone onorigin/mainand passes here.
All four are one mechanism: an eager import was carrying side effects, and making
it lazy relocated them.
Recorded, not done — both on UN-4136 with mechanism and repro
- Make a wholesale cloud-plugin import failure loud at boot. Today a broken wheel
boots clean and surfaces per-request asNo executor registered with name 'table'. - Fold the ~30 hand-copied registration guards into a shared test helper, so
omission becomes impossible rather than merely discouraged.
Honest accounting of the eight rounds
Rounds 1–3 found real defects in the change. Rounds 4–8 were almost entirely my
own fixes being wrong — twice removing a guard instead of narrowing its claim,
three times rewriting one comment into a new falsehood. A better first pass would
have needed half the rounds. The loop caught all of it, which is the point, but the
round count is not a boast.
|
 |
Unstract test resultsPer-group results
Critical paths
|
CI state — the test tiers ran, and under the scheduler that mattersFlagging this because the check rollup read They were gated on draft state, not broken. if: needs.changes.outputs.relevant == 'true' && github.event.pull_request.draft == falseSo the tiers were correctly skipped for the eight self-review rounds this PR spent
Step-level: Why the unit tier is the right check for this change specifically. It runs That is precisely the scheduler this PR had to be hardened against. Making the Correcting one claim I made in an earlier roundRound 5's write-up narrowed an xdist claim on the grounds that "there is no What CI does not cover, unchanged from the PR body
|
What
Each
pg_queue_consumerchild paid ~9s of one-time lazy initialisation on its first task. The supervisor preforks 20 children per pod, so that is ~170 CPU-seconds per pod, spent during serving rather than at startup.This stops the file_processing worker from importing the executor stack at all.
Why it costs so much
workers/file_processing/structure_tool_task.pyhas three function-local imports from theexecutorpackage — two for string constants, one for a shim:None of them needs the executor registry. But every import under the
executorpackage booted the whole thing:So line 321 — which fires first, in step 1 of
_execute_structure_tool_impl— dragged inLegacyExecutorand every adapter behind it, to construct aStreamMixinwrapper. Measured onorigin/main, a py3.12 venv, with the file_processing worker already imported as the baseline:Because the imports are function-local, the cost is deferred to each child's first task — which is why CAST AI's
startup: {periodSeconds: 120}window cannot exclude it. That window is anchored to pod start; on staging, pod...-cg9ntstarted ~07:45 IST and its children initialised at 11:08, 3.4 hours later.Consequences this fixes
maxReplicas 8that then unwinds.minReplicaseven with a 90-message backlog.The change
The ticket proposed three variants. Findings ruled out two of them, and the reasoning is recorded in full on UN-4136:
PromptServiceConstantsout from underexecutor.executors) cannot work — line 321 pulls the package regardless, viaexecutor/__init__.py.pg_queue_consumer/supervisor.py's_run_childstates it outright: "The worker import (and any connections it opens) happens HERE, in the child — never inherited across the fork — so each process owns its own connections."So this removes the cost instead of relocating it:
executor/package initcelery_applazily (PEP 562 module__getattr__).from executor import celery_appstill works.executor/executors/package initregister_all().LegacyExecutorstays reachable from the namespace, lazily.executor/worker.py,executor/tasks.pyregister_all()directly.Both entrypoints already carried an explicit
import executor.executors # noqa: F401for exactly this purpose, with comments saying so — this makes an existing intent explicit rather than changing it.structure_tool_task.pyis untouched: all three of its imports become cheap on their own.Result
Same harness, same environment, before vs after:
legacy_executorloaded2.33s rather than 8.5s locally because the page cache is warm and the cloud entry-point executors aren't installed on this machine — those were the +1.15s and +0.27s segments in the pod. The stack is no longer imported at all, so the whole segment goes away regardless of its size.
Tests
tests/test_executor_import_isolation.py(new) pins the import graph, not a duration — a timing assertion would be flaky on CI, while the thing that actually regresses is an eager import creeping back into either package init. Each case runs in a fresh interpreter so the rest of the suite cannot pre-import the stack and mask it.Both cases were confirmed failing on
origin/main:The pre-existing
tests/test_executor_registration.py— which already covers this ticket's "registry populated before the first task is claimed" bar, via the PG-consumer bootstrap path — still passes.tests/test_plugin_loader.pyis updated to the explicit-registration contract, plus a case worth calling out.ExecutorRegistry.registerraisesValueErroron a name already registered, and every cloud plugin importsLegacyExecutor(e.g.lookup_enrichment/src/base.py:13), so a plugin pulled in ahead ofregister_all()fires that decorator first.register_all()is safe because it registers through module imports rather than an explicitregister(...)call, sosys.modulescaching stops the decorator running twice — but that is load-bearing and now pinned bytest_register_all_tolerates_legacy_executor_already_imported. Had it been written the other way, the second call would raise and take the executor worker down at startup.Not verified here
End-to-end convergence on staging (9.08s -> ~0.39s local-work segment). Needs a deploy; this PR verifies the import graph and the local import cost.
Cloud entry-point executors are not installed locally, so registration is verified for
legacyonly — worth exercising on staging.What is verified: none of them relied on the removed side effect. All 8 entry points in the
unstract.executor.executorsgroup (table,lookup_test,single_pass_extraction,simple_prompt_studio,smart_table,agentic,line_item,agentic_table) were swept for bare-package imports, including function-local ones — that being how the original bug hid. There are none. Every spelling used across all 8 is a submodule import (executor.executors.constants,...legacy_executor,...answer_prompt,executor.executor_tool_shim, and six more), so nothing depended onimport executor.executorsregistering anything.PR_TYPE
FIX, on the QA test in the workspace
CLAUDE.md. This is not infra or CI — it changes the registration path for the executors that do all extraction work. Ifregister_all()were wrong, extraction fails outright withNo executor registered with name 'legacy'. That is squarely something QA must exercise against a running deployment, and the cloud entry-point executors above are the specific thing to exercise.Out of scope, as recorded on the ticket: the CAST AI
stabilitypolicy mis-assignment.