Skip to content

UN-4046 [MISC] Roll the PG queue out of the pg_queue_enabled feature flag - #2262

Open
muhammad-ali-e wants to merge 11 commits into
mainfrom
feat/UN-4046-MISC_pg-queue-out-of-feature-flag
Open

UN-4046 [MISC] Roll the PG queue out of the pg_queue_enabled feature flag#2262
muhammad-ali-e wants to merge 11 commits into
mainfrom
feat/UN-4046-MISC_pg-queue-out-of-feature-flag

Conversation

@muhammad-ali-e

Copy link
Copy Markdown
Contributor

What

Removes the pg_queue_enabled feature flag and makes the PostgreSQL queue the
unconditional default — in code, in environment defaults, and in the local Docker stack.

  • workflow_manager/workflow_v2/transport.py deleted. resolve_transport() was
    entirely flag logic; its three callers now use the PG constant directly. The
    transport wire field is kept, hardcoded to pg_queue — workers read it at eight
    sites, and removing it is separate work.
  • Remaining flag reads gone: prompt_studio_core_v2 task_status, scheduler/ownership,
    both pg_queue/executor_rpc factories, and the shared resolve_pg_transport /
    RoutingExecutionDispatcher in unstract/workflow-execution.
  • Two defaults flipped to PG. Neither moves with the flag, and each independently
    leaves a producer with no consumer:
    • select_backend() read WORKER_PG_QUEUE_ENABLED_TASKS, an allow-list set
      nowhere
      (commented out in sample.env), so every dispatch without an explicit
      backend= went to Celery. The live case is api-deployment/tasks.py dispatching
      process_batch_callback_api. The allow-list is deleted along with its plumbing.
    • PG_SCHEDULER_ENABLED default falsetrue. It gates whether the PG scheduler
      owns a pipeline's cron; left off with Beat disabled, nothing fires schedules at all.
  • WORKER_BARRIER_BACKEND is deliberately not touched — _barrier_for_transport()
    returns a PgBarrier whenever the transport is PG, regardless of that variable.
  • docker/docker-compose.yaml is PG by default. The eleven Celery worker services are
    removed, profiles: [pg-queue] is dropped from the PG consumers, and six queues that
    had no PG consumer are closed (file_processing_priority, celery_callback,
    celery_executor_table, _smart_table, _simple_prompt_studio, _lookup_test).
    worker-log-history-scheduler-v2 (a bash loop, not Celery) and
    worker-log-stream-consumer stay.
  • Dead Celery-coupled code removed where the flag removal made it trivially reachable:
    four callerless webhook views and their serializers, submit_file_batch_for_processing
    end to end, and three unused constants.
  • Retry jitter in the workers' HTTP client is now unconditional. It was gated so the
    Celery flow kept an exact exponential cadence; it is transport-agnostic hardening and
    the jittered cadence is what integration has run under load.

Why

QA passed 100% PG on integration, so the flag has served its purpose and staging is next.

It has also become a liability rather than a safety net. On 2026-08-28 a routine node
rebalancing eviction restarted Flipt on integration; resolve_transport failed closed to
Celery and published to RabbitMQ, where a 100%-PG topology has no consumers. Two
executions stranded permanently — the undispatched sweep excludes them (task_id is set)
and stuck-execution recovery excludes them (the file never reached a terminal state). With
no flag there is no Flipt call in the dispatch path and this class of strand disappears.

The two default flips matter for the same reason. A producer without a consumer does not
merely lose the work: the messages accumulate in RabbitMQ until it hits its memory
high-watermark and starts blocking publishers, which degrades everything still using the
broker.

How

Deletion-led. Almost every hunk removes a branch rather than adding code — the current
flag-on behaviour becomes the only behaviour, with no business-logic change. Net
−2 000-odd lines.

Tests were reworked per assertion rather than wholesale: anything pinning a payload, queue
or error contract is kept; anything that only tested the branch choice is removed, with
the reason recorded where it was. Two examples:

  • TestDispatchAttachesFairness patched current_app and read send_task.call_args.
    That branch is now unreachable, so the cases assert the same property against the PG
    path — fairness must reach the transport without being folded into the business kwargs,
    which on PG means the payload slot and the org_id / priority columns the dequeue
    orders by. One case keeps the Celery branch covered through an explicit backend=
    override, its only remaining reachability.
  • TestBackoffJitter's two flag-off cases are gone; they asserted the exact exponential
    cadence the Celery flow kept, and the band assertion already pins the result from both
    sides.

A new zero-producer canary (TestNoCeleryProducer) asserts that no dispatch can
resolve to Celery, so a future call site cannot silently reintroduce a broker publish.

Can this PR break any existing features?

The intended behaviour change is that execution dispatch no longer has a Celery path.
Everything else is preserved, and the two risks are deployment-shaped rather than
code-shaped:

  • Deploy order is load-bearing and one-directional. The PG worker fleet must be up in
    the same release as this code. Deploying this against a topology whose PG consumers are
    off would enqueue to queues nothing drains — no fallback, no reaper recovery, no error.
    The chart's partial-fleet guard refuses the half-configured version at render time.
  • There is no runtime rollback any more. Reverting means redeploying the previous
    image and chart, not flipping a flag.

Not affected: queue payloads and task names are unchanged; the transport wire field
still travels (hardcoded to pg_queue), so workers need no coordinated change; the Celery
library and the @app.task registry stay permanently, because the PG consumer is a
Celery app and resolves work through self._app.tasks.get(name).

step_execution (single-step workflow execution) now raises unconditionally. It has had
no UI for some time — both live front-end call sites pass isStepExecution=false — so it
is reachable only by a direct API call, and any organisation already on PG hits this
today. Deleting the endpoint outright is tracked separately as UN-4051.

Database Migrations

None.

Env Config

  • WORKER_PG_QUEUE_ENABLED_TASKSremoved. Delete it from any environment that sets
    it (it was commented out in sample.env and set nowhere).
  • PG_SCHEDULER_ENABLED — default changes falsetrue.
  • No new variables.

Notes on Testing

Run locally against a live Postgres:

Backend suite 1005 passed, 29 skipped, 0 failed
Workers suite 1501 passed, 1 skipped, 0 failed
pre-commit, changed files clean
ruff (0.3.4, changed files) 24 → 23 errors vs main

The backend suite was run in a clean worktree. A working checkout with git-ignored
enterprise plugins present produces unrelated import failures that look like regressions.

Two checks beyond the suites, because no CI job covers them:

  • docker/docker-compose.yaml — parses with no profile into twelve worker services,
    all PG except the two log services; zero Celery workers, zero beat.
  • Queue coverage — every queue with a PG consumer in the local stack has one in the
    deployment chart. Zero gaps in that direction.

Related Issues or PRs

UN-4046, under epic UN-3445. Follow-up filed as UN-4051 (delete the deprecated
single-step execution endpoint).

Checklist

  • I have added an appropriate PR title and description
  • I have read and understood the Contribution Guidelines
  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented on my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have checked my code and corrected any misspellings

muhammad-ali-e and others added 8 commits August 31, 2026 11:20
… endpoints

Groundwork for taking the PG queue out of the pg_queue_enabled flag: delete the
Celery-coupled code that has no callers, so the flag removal that follows does not
have to reason about it.

Removed, all verified callerless across both repos including tests:

- notification_v2 webhook cluster: WebhookSend / WebhookStatus / WebhookBatch /
  WebhookBatchStatus views and their four routes. One dead chain -- Send/Batch
  celery_app.send_task, Status/BatchStatus poll the returned ids via AsyncResult.
  notification_v2/internal_views.py now imports no Celery at all.
- The five serializers those views owned, definition-only once they went.
- submit_file_batch_for_processing: backend view, route, and both client methods
  (internal_client only delegated to execution_client; nothing called the facade).
- Three WEBHOOK_SEND constants -- defined 3x, referenced 0x.

Kept: WebhookInternalViewSet (CRUD), WebhookTestAPIView, WebhookMetricsAPIView --
none touch Celery.

Also unconditional now: _backoff_with_jitter no longer reads pg_queue_enabled.
Jitter is transport-agnostic HTTP-retry hardening and the jittered cadence is what
integration has run under load; with the flag going away the flag-on behaviour is
the one that stays.

Verified: manage.py check clean (catches the deleted routes), notification_v2 13/13,
workflow_manager 158 passed. The one workflow_author failure is pre-existing --
identical on clean origin/main. ruff check reports the same 8 errors as main, none
in the touched lines.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
First two of the eight pg_queue_enabled read sites. Both seams had the same shape
-- resolve_transport() then if is_pg_transport(): PG else: celery_app.send_task().
The flag is going away, so the PG body becomes the whole function and the Celery
branch is deleted.

pipeline_dispatch: dispatch_pipeline_trigger() drops its celery_app parameter (the
only consumer was the deleted branch) and its caller drops the now-unused
celery_service import.

notification_dispatch: same, plus the DispatchResult NamedTuple and its
PG_TRANSPORT/CELERY_TRANSPORT labels. Those existed solely so the dispatch metric
could tell the transports apart during a ramp -- with one transport the field is a
constant, so the seam now returns the task id and the log line drops transport=.
The two forced kwargs (max_retries=0, raise_on_final_failure=False) become
unconditional; they are what the eager apply() consumer needs, not a PG-branch
quirk.

ALSO ADDS worker-pg-notification TO docker-compose. OSS compose had nine PG
consumers and none for the notifications queues, so making this seam PG-only
without it would strand every buffered webhook in local dev -- nothing sweeps
pg_queue_message. Mirrors the chart's workerPgNotification (5 queues, VT 300s,
health-stale 360s) and rides the existing pg-queue profile.

Tests reworked rather than deleted wholesale -- each assertion judged on whether it
protects behaviour or just the flag:
- kept, de-flagged: payload/queue contract, transient-vs-permanent error split,
  the forced terminal-branch kwargs, UUID coercion, and (reshaped) the missing-org
  case, which still pins org_id="" on the row since enqueue_task types it as str.
- deleted: routes-to-celery, resolve_transport-entity-bucketing, and
  args-identical-on-both-paths -- there is no second path to be identical to.
9 removed, 8 kept. notification_v2 + pipeline_v2: 19 passed.

Verified: manage.py check clean; ruff clean on the changed lines (the one TC006 in
internal_api_views.py is pre-existing, confirmed on origin/main); compose parses
and now lists 10 pg services.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Third of the eight flag sites, and the largest. resolve_transport() was entirely
flag logic -- org guard, FLIPT_SERVICE_AVAILABLE guard, the Flipt read, and a
fail-closed except, all resolving to "celery" or "pg_queue". With the flag gone it
collapses to a constant, so the module and its test file are deleted outright.

Its three callers:

- _dispatch_orchestrator_task: the 3-line celery_app.send_task else goes; the PG
  enqueue body is now the whole function, and the transport parameter with it.
- _record_dispatch_handle: the update_execution_task else goes. task_id is no
  longer written by any dispatch path -- it stays NULL, which is what the
  undispatched sweep and recover_stuck_pg_executions already assume for PG rows.
- internal_api_views.create_workflow_execution: no branch, the value went over the
  wire to the scheduler worker. Now returns the constant.

The transport WIRE FIELD is deliberately kept and hardcoded to PG_QUEUE in both
places. Workers still branch on it at eight sites (general, api-deployment,
scheduler, pg_barrier, execution service); removing the field is part of the
deferred Celery-branch cleanup, not this change.

step_execution now refuses unconditionally. Its polarity was inverted -- the
flag-ON branch was the raise -- so keeping flag-on behaviour makes the refusal
permanent. That is not a capability loss: the path is deprecated and UI-unreachable
(its own comment: "the step buttons are gone and both live call sites pass
isStepExecution=false"), a frontend grep finds nothing, and any org on PG already
hits this. Deleting the endpoint is tracked separately.

Tests, judged per assertion:
- deleted test_transport.py entirely (the whole suite tested the flag).
- test_step_execution_transport.py keeps the one assertion that matters -- that
  nothing reaches the chord -- and drops "celery still runs it" and "transport is
  resolved on the execution id".
- test_dispatch_orchestrator / test_record_dispatch_handle: dropped the two
  celery-branch tests, de-parameterised the rest.

Verified: manage.py check clean; workflow_manager 123 passed, pipeline_v2 +
notification_v2 green (142 total). The one workflow_author failure is pre-existing
on origin/main.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nsport

Closes the last five pg_queue_enabled read sites and flips the two Celery-by-default
switches that survive flag removal.

Flag reads removed (keep flag-on, delete flag-off):

- prompt_studio task_status: the AsyncResult tail goes, the PgTaskResult branch is
  now the whole method. Nothing writes a Celery result backend on this path, which
  is why the gate existed (UN-3693).
- scheduler/ownership.resolve_schedule_owner: collapses to pg_scheduler_enabled().
  The env gate STAYS and is deliberately not folded away -- reconcile_ownership_for
  disables the Beat PeriodicTask whenever a schedule is PG-owned, so a deployment
  without a running PG scheduler must still be able to keep Beat, or the pipeline
  has no firer at all.
- Both get_executor_dispatcher factories return PgExecutionDispatcher directly.
  celery_app is kept as an accepted-and-ignored parameter: ~20 call sites across
  both repos pass it, and changing that signature is a cross-repo edit, not part of
  removing a flag.
- Shared executor_rpc: resolve_pg_transport, RoutingExecutionDispatcher, the
  _CeleryDispatcher Protocol and PG_QUEUE_FLAG_KEY all deleted. The two surviving
  annotations retarget to PgExecutionDispatcher.

Defaults flipped to PG -- each of these was a producer with no consumer, which does
not merely lose work but accumulates in RabbitMQ until it blocks publishers:

- select_backend() now returns PG unconditionally and the
  WORKER_PG_QUEUE_ENABLED_TASKS allow-list is deleted (code + sample.env). It was
  set in NO environment, so every dispatch without an explicit backend= went to
  Celery. That is a live producer today: api-deployment/tasks.py:755 dispatches
  process_batch_callback_api with no override. resolve_backend's per-call override
  survives -- pg_barrier passes it explicitly and stating the transport at the
  dispatch site reads as intent rather than reliance on a default.
- PG_SCHEDULER_ENABLED now defaults on. Opting OUT is the deliberate act.

NOT flipped: WORKER_BARRIER_BACKEND. It looked like a third switch but selects the
barrier only on the CELERY transport -- _barrier_for_transport() returns a fresh
PgBarrier() whenever is_pg_transport(transport), "regardless of
WORKER_BARRIER_BACKEND". With transport hardcoded to PG it is dead by consequence
and goes with the deferred cleanup.

Tests judged per assertion, not deleted wholesale. Kept: payload/queue contracts,
the transient-vs-permanent error split, PG-enqueue-never-falls-back, per-thread
client reuse, the log-once property, and the scheduler gate in BOTH directions.
Deleted: every celery-branch case, the resolve_pg_transport suite, the allow-list
parsing/observability suites, and the RoutingExecutionDispatcher isinstance
assertions. Two reconcile helpers now drive ownership through
resolve_schedule_owner instead of the flag.

Verified in a CLEAN worktree, not this checkout: backend 1005 passed / 0 failed.
That matters -- my working copy carries a git-ignored backend/plugins/authentication/
auth0/ from earlier enterprise dev-testing which imports the cloud-only
pluggable_apps module and 500s six auth-touching tests. Those are an artefact of
this machine, and an earlier claim in this branch that test_workflow_author was
"pre-existing on main" was wrong for the same reason -- it passes clean.

ruff: 13 auto-fixed; the one remaining (N801 TestDryRunReaches_Everything) is
pre-existing on origin/main and outside this diff.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ucer canary

docker-compose: the eleven Celery worker services are deleted (celery-beat,
general-v2, executor-v2, file-processing-v2, callback-v2, api-deployment-v2,
notification-v2, scheduler-v2, ide-callback, log-consumer-v2, metrics) and the
`pg-queue` profile is removed from the eleven PG consumers, so a plain
`docker compose up` now starts the PG stack. Kept: log-history-scheduler-v2 (a bash
loop, not Celery) and log-stream-consumer. celery-flower stays behind its existing
`optional` profile.

CLOSED THE QUEUE-COVERAGE GAP. Six queues the chart's PG fleet drains had no OSS
consumer, so PG-only dispatch would have stranded them in local dev exactly as the
notifications queue would have: file_processing_priority and celery_callback were
missing from the fileproc/callback consumers, and the executor consumer covered
three of the chart's seven executor queues. Diffing the two queue lists now leaves
only agentic_callback / agentic_studio / bulk_download / subscription, which are
cloud-only workers with no OSS equivalent.

Auxiliary files retargeted, because deleting a service that an override still names
does not error -- compose CREATES a half-defined service from the override alone:
workers.sh (service list, valid short names, and worker_service_name now maps to
worker-pg-* with two special cases), compose.debug.yaml (debugpy port map),
sample.compose.override.yaml, README.md. Verified every name they reference exists
in the base file. The five names still dangling in sample.compose.override.yaml
(tool-sidecar, worker, worker-file-processing, worker-file-processing-callback,
worker-logging) were already stale on origin/main -- checked, not introduced here.

test_queue_backend_seam.py inverted. It existed to prove dispatch() produced
byte-identical current_app.send_task calls to the raw Celery idiom, so that adding
PG routing could be shown to preserve the Celery default -- the default this change
removes. Replaced with TestNoCeleryProducer, which asserts no call site can resolve
to CELERY and that dispatch never calls send_task. That is the acceptance criterion
worth guarding: a producer without a consumer does not merely lose the work, the
messages accumulate in RabbitMQ until it blocks publishers.

Its second layer is kept unchanged -- @worker_task must still register with the
Celery app, which is NOT vestigial: the PG consumer resolves work via
self._app.tasks.get(name), so the Celery registry stays load-bearing even though
the Celery transport is gone.

Verified: `docker compose config` valid; `bash -n workers.sh` clean; every
workers.sh entry present in compose; seam/routing/dispatch/executor_rpc suites 60
passed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two test classes asserted a premise that inverted, and failed on a mock that
was never called rather than on the behaviour they describe:

- test_fairness_key TestDispatchAttachesFairness patched current_app and read
  send_task.call_args — the Celery branch. select_backend returns PG
  unconditionally now, so that branch is unreachable without an explicit
  backend= override. Rewritten against the PG path: fairness must reach the
  transport without being folded into the business kwargs, which on PG means
  the payload slot AND the org_id/priority columns the dequeue orders by
  (a payload-only fairness would be inert). One case keeps the Celery branch
  covered via the override, since that is now its only reachability.
- test_pg_finalization_fixes TestBackoffJitter patched the flag that gated
  jitter. The two flag-off cases are removed — they asserted the exact
  exponential cadence the Celery flow kept, and there is no Celery flow and no
  Flipt call. The band assertion already pins the result from both sides.

Docstrings that described the flag as live behaviour, corrected in place:
queue_backend/dispatch.py (defaulted to Celery via an allow-list set nowhere),
prompt_studio_helper._get_dispatcher, notification_v2._org_identifier (it
still needs the org string — for the PG row's fairness column, not for a
Flipt decision), and the two sweep comments that pointed at resolve_transport.

No behaviour change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…re-commit

Running pre-commit over the branch (every commit so far used --no-verify)
surfaced one real defect and six formatting fixes.

The defect: workflow_helper.py:717 still interpolated `transport` into the
post-dispatch bookkeeping log after 31fff43 removed the parameter. That is a
NameError on the exception path — an already-dispatched execution would have
raised inside its own error handler instead of returning EXECUTING, turning a
recoverable bookkeeping failure into a 500. No test covers that branch and ruff
(F821) is what caught it; the message now names the PG queue directly.

The rest are pycln/ruff-format: one unused MagicMock import, one unused patch
import left by the jitter-test rework, and four import-block orderings.

Suites re-run after the edits.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ocal stack

Found by running the stack, which no CI job does.

Deleting worker-log-consumer-v2 left the local stack with a Redis log consumer
and zero publishers pointed at it: LOG_TRANSPORT was set on
worker-log-stream-consumer — the CONSUMER — and nowhere else. Every publisher
kept the "celery" default and would have written execution logs to
celery_log_task_queue on RabbitMQ, which this stack no longer drains. That fails
silently in both directions: LogPublisher swallows its errors, so the symptom is
execution logs that stay empty, while the queue grows unbounded toward the
broker's memory high-watermark. The requirement was already written in the
compose comment above the consumer; the change just did not honour it.

Now set on all fourteen publishers — backend, runner and the twelve workers.

In `environment:` rather than essentials.env on purpose. That file is git-ignored
and generated from sample.essentials.env, and copy_or_merge_envs only merges new
keys when --only-env or --update is passed, so every existing checkout would have
silently kept the old default on a plain `./run-platform.sh`. An `environment:`
entry is version-controlled and wins over env_file, so it needs no regeneration.

Also drops the "sits in the pg-queue profile" note on the consumer — profiles are
gone from this file.

Verified on the running stack: all 14 resolve to redis via `compose config`,
containers report LOG_TRANSPORT=redis, consumers start clean (0 ERROR-level
lines), and the two non-zero RabbitMQ queues are pre-existing backlog that stayed
flat across the run.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@muhammad-ali-e

Copy link
Copy Markdown
Contributor Author

Ran the local Docker stack while preparing this — worth recording, since no CI job builds or runs docker/docker-compose.yaml and that file is where eleven services were deleted.

It found a real defect, now fixed in 3e962ae. Removing worker-log-consumer-v2 left the stack with a Redis log consumer and zero publishers pointed at it: LOG_TRANSPORT was set on worker-log-stream-consumer — the consumer — and nowhere else. Every publisher kept the celery default and would have written execution logs to celery_log_task_queue on RabbitMQ, which this stack no longer drains. Silent in both directions: LogPublisher swallows its errors, so the symptom is execution logs that stay empty while the queue grows toward the broker's memory high-watermark. The requirement was already written in the compose comment above the consumer; the change just did not honour it.

It is now set on all fourteen publishers — backend, runner and the twelve workers — in environment: rather than essentials.env. That file is git-ignored and generated, and copy_or_merge_envs only merges new keys on --only-env/--update, so every existing checkout would have silently kept the old default on a plain ./run-platform.sh.

What the run verified

  • Stack comes up with no profile: twelve worker containers, all PG except worker-log-history-scheduler-v2 (a bash loop) and worker-log-stream-consumer. Zero Celery workers, zero beat.
  • Every PG consumer starts and polls its queues. The six previously-uncovered queues are now drained — file_processing_priority on worker-pg-fileproc, celery_callback on worker-pg-callback, and celery_executor_table / _smart_table / _simple_prompt_studio / _lookup_test on worker-pg-executor.
  • 0 ERROR-level lines across all workers. (One startup line in worker-log-stream-consumer is a RabbitMQ race that retried to ready.)
  • All 21 named RabbitMQ queues at 0 messages except two carrying pre-existing backlog from earlier sessions; both stayed flat across the run.

What it could not verify. Docker Hub is unreachable from this machine (TLS handshake timeout), so images containing these changes could not be built — the containers ran a prebuilt worker-unified image. This exercises the compose wiring (service set, queue assignments, env), not the Python changes; those are covered by the suites. A full ETL end-to-end still needs to happen on integration.

@muhammad-ali-e muhammad-ali-e left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Standardized review — BLOCK

Mode: INITIAL · reviewed at 3e962aec8 · 16/16 lenses · ten specialist agents across both repos, findings deduplicated and re-rated against the shared rubric, every Critical and most Highs independently verified.

Summary (both repos): Critical 2 · High 12 · Medium 21 · Low 8. This PR carries the Critical.

Findings are in the body rather than as inline comments by necessity: most of the severe ones sit on lines this diff does not touch (the headers= call sites, DEFAULT_WORKFLOW_TRANSPORT, run-worker.sh, the ownership.py docstring), and GitHub rejects inline anchors outside a diff hunk. Every finding carries file:line.


CRITICAL — every document extraction raises TypeError

workers/file_processing/structure_tool_task.py:501, 544, 761unstract/workflow-execution/src/unstract/workflow_execution/executor_rpc.py:135

get_executor_dispatcher() now returns PgExecutionDispatcher, whose dispatch(self, context, timeout=None) has no headers parameter and no **kwargs. The deleted RoutingExecutionDispatcher.dispatch(context, timeout, headers) absorbed it. Three live call sites still pass headers=; :544 is the main structure_pipeline path.

Reproduced directly:

TypeError: PgExecutionDispatcher.dispatch() got an unexpected keyword argument 'headers'

Caught by the wrapper at :231 and surfaced as ExecutionResult.failure, so every document fails extraction with a message naming a Python signature rather than the cause. dispatch_async lost headers too.

It ships green because every dispatcher double is a bare MagicMock() with no spec= (workers/tests/test_structure_tool_pipeline.py:177 and ~11 siblings), so the suite cannot see a dispatcher signature change at all.

Fix: drop the three headers= arguments (PG carries fairness in the enqueue payload), and add spec=PgExecutionDispatcher to the doubles so this class of drift fails a test instead of production.


HIGH

H-a · The safe default inverted without movingunstract/core/src/unstract/core/data_models.py:219 (not in this diff)
DEFAULT_WORKFLOW_TRANSPORT = WorkflowTransport.CELERY.value, and normalize_transport still fails closed to it, at eight consumer signature sites. Producers now always send pg_queue, but a payload missing the field selects CeleryChordBarrier via _barrier_for_transport and skips the PG orchestration-claim and terminal-redelivery guards — publishing the fan-out to RabbitMQ, which this PR empties of consumers. No barrier row is created, so the reaper is blind and the execution sits EXECUTING forever. Reachable on a rolling deploy: a not-yet-upgraded backend returns transport: "celery", select_backend routes to PG anyway, and the worker fans out over the dead broker. One-line fix: flip the constant to PG_QUEUE in this PR.

H-b · The debug stack drains nothingdocker/compose.debug.yaml:71-186
15 celery -A worker invocations, 0 pg-queue-consumer. Service keys were renamed to worker-pg-*; the bodies were not. As an override this replaces command: ["pg-queue-consumer"], so the documented debug flow turns every PG consumer back into a Celery worker bound to RabbitMQ. Executions hang with empty logs — the exact symptom this migration exists to remove.

H-c · PG_SCHEDULER_ADOPT_PERIODICS is set nowhere — repo-wide grep returns zero
converge_pg_scheduler adopts dashboard_metrics.* only with --periodics, which backend/entrypoint.sh:66 derives from that variable. This PR deletes celery-beat and worker-metrics while those periodics stay Beat-owned, so nothing fires them. The cloud chart refuses this combination at render time; the OSS compose has no equivalent guard.

H-d · run-worker.sh has no pg-notification roleworkers/run-worker.sh:61-93
No notification role exists at all, and three queue lists drifted from compose (celery_callback, file_processing_priority, and four celery_executor_*). ./run-worker.sh pg silently drops every buffered webhook — _enqueue_pg succeeds, nothing drains.

H-e · Operator-facing docs invert the most consequential behaviour changebackend/entrypoint.sh:36-45, backend/scheduler/ownership.py:14-25
Both still describe two gates with PG_SCHEDULER_ENABLED defaulting off, and state that a restart "moves nothing". It is now one gate defaulting on, so the first --migrate start hands every mirrored schedule to PG and disables Beat's rows. entrypoint.sh even records that a previous version of itself made this same mistake.


MEDIUM / LOW — grouped (29 findings)

Lens 16 dominates. Docstrings still promising a Celery branch: both pg_queue/executor_rpc.py module headers, dispatch() (workers/queue_backend/dispatch.py:93-104, incl. a reference to the deleted _log_allow_list_once), workers/queue_backend/__init__.py:27-34 ("observable but inert"), workers/queue_backend/pg_queue/__init__.py:19-20, workers/queue_backend/pg_queue/README.md:6-7,56-59 (tables the deleted flag and allow-list as live knobs), docker/sample.env:147-151, docs/local-dev-setup-executor-migration.md:132-145 (commands for deleted services). Half-edits: docker/docker-compose.yaml:430 claims workers get LOG_TRANSPORT "via essentials.env" — there are 14 explicit per-service entries and 0 in that file, and the correct rationale is written 370 lines earlier; :458-459 still calls worker-pg-executor "Dark until the pg_queue_enabled gate is flipped" when it is now the only executor. Orphans: backend/pg_queue/flags.py (zero importers, docstring cites the deleted transport.py), and the PG_QUEUE_FLAG_KEY comment now heading EXECUTE_TASK at executor_rpc.py:47-49. An operator-facing RuntimeError at workers/queue_backend/pg_barrier.py:475 still says "before enabling PG transport (pg_queue_enabled)".

Correctness / quality. backend/backend/internal_api_constants.py:68webhook_send() -> str lost its body and returns None. docker/sample.compose.override.yaml:299 builds worker-pg-metrics from backend.Dockerfile, which rejects pg-queue-consumer and crash-loops. Dead assignment at workflow_helper.py:1023. QueueBackend.CELERY is unreachable by construction yet dispatch() still executes send_task for it — an unreachable branch that silently drops messages is worse than one that raises. api-deployment/tasks.py:755 now routes to PG without PG_TRANSPORT_CALLBACK_KWARG, so the callback's duplicate guard is skipped (enclosing comment argues the branch is unreachable — worth confirming).

Testing. Dispatcher doubles lack spec= — this is why the Critical shipped green. test_flipt_is_never_consulted_while_the_gate_is_off (backend/scheduler/tests/test_pg_schedule_ownership.py:296) now has zero assertions. Two test_step_execution_transport cases were deleted, one of which existed specifically to pin the guard placement this PR moved. test_uuid_job_id_is_stringified deleted though the coercion is live. TestNoCeleryProducer overclaims: it enumerates six names against an unconditional return QueueBackend.PG and inspects no call site, so a reintroduced Celery publish would ship green in all three realistic forms.


Lens checklist (16/16)

# Lens Result
1 Spec & intent See Critical, H-a — "no business-logic change" does not hold
2 Architectural fit See Medium — QueueBackend.CELERY still executable into a dead broker
3 Correctness See Critical, H-a, webhook_send
4 Security Clean — assessed directly: no authn/authz, tenant-scoping or secrets surface
5 Data integrity See H-c. No migrations; sweep predicate verified correct for pre- and post-upgrade rows
6 Concurrency See H-a. Diff adds no concurrency surface — barrier selection untouched
7 API & contract See Critical (dispatcher signature), removed internal routes
8 Reliability See H-a, H-b
9 Performance & cost Clean — assessed directly, no hot-path change
10 Observability LOG_TRANSPORT defaults to celery in pubsub_helper.py:42 and appears in no sample.env
11 Operational safety See H-b, H-c, H-d — heaviest lens on this PR
12 LLM/agent N/A — verified: zero added lines touching prompts, model config, tool schemas or evals
13 Testing See testing group
14 Dependencies & build Clean — verified: no dependency, lockfile or Dockerfile changes
15 Code quality See Medium/Low
16 Doc & comment accuracy Largest category by volume

Open questions

  1. In-flight RabbitMQ work at cutover. Production runs Celery today. This release deletes the Celery Deployments, so claimed tasks drain but unclaimed queued messages have no consumer and strand permanently. Since the new code publishes nothing to RabbitMQ, keeping the Celery workers enabled for one release drains the backlog at no cost — the same posture already taken for workerLogConsumerV2.
  2. Should the deprecated single-step endpoint be deleted now rather than left as an unconditional error? (UN-4051 exists.)

Assumptions

Reviewed the committed head 3e962aec8. helm-unittest 0.5.1 / helm 3.15.3, the pair CI pins.

…hs, and the doc sweep

CRITICAL — every document extraction raised TypeError.
get_executor_dispatcher() returns PgExecutionDispatcher, whose dispatch() has no
`headers` parameter and no **kwargs; the deleted RoutingExecutionDispatcher
absorbed it. Three live call sites in structure_tool_task.py still passed
`headers=`, including the main structure_pipeline path. Reproduced:
"TypeError: PgExecutionDispatcher.dispatch() got an unexpected keyword argument
'headers'", surfaced as ExecutionResult.failure by the wrapper — so every document
failed with a message naming a Python signature.

Nothing is lost by dropping them: the router never forwarded headers to the PG
path (its own docstring said so), and PG carries org routing in the enqueue
payload (transport.enqueue(..., org_id=...)). _fairness_headers() and its imports
go with them.

It shipped green because all 12 dispatcher doubles were bare MagicMock(), which
accepts any keyword. Switched to create_autospec — and NOT MagicMock(spec=...),
which was verified to still accept `headers=`. Mutation-checked: re-adding
`headers=` to the pipeline dispatch now turns 11 tests red.

HIGHS
- compose.debug.yaml had 15 `celery -A worker` invocations and 0 PG consumers.
  The service keys were renamed to worker-pg-*; the bodies were not. As an
  override it REPLACES `command: ["pg-queue-consumer"]`, so the documented debug
  flow turned every PG consumer into a RabbitMQ worker and the stack drained
  nothing. Rewritten to wrap `python -m pg_queue_consumer` under debugpy, with the
  queue set deliberately NOT restated — the module reads it from the base file's
  env, and restating it is what let this drift. Verified: the merged debug stack
  now has zero Celery workers.
- PG_SCHEDULER_ADOPT_PERIODICS was set nowhere in the repo. converge_pg_scheduler
  adopts dashboard_metrics.* only with --periodics, so deleting celery-beat left
  those rows Beat-owned with Beat gone. Added to the backend service.
- run-worker.sh had no pg-notification role AND no pg-ide-callback role, plus
  three queue lists drifted from compose. A host-run `pg` fleet silently dropped
  every buffered webhook. Synced against `docker compose config`: 23 queues, exact
  parity both directions.
- entrypoint.sh and scheduler/ownership.py told an SRE that a restart "moves
  nothing" and that PG_SCHEDULER_ENABLED defaults off behind a second Flipt gate.
  It is one gate defaulting ON, so the first --migrate start moves every schedule.
  entrypoint.sh even recorded that a previous version made this same mistake.

DOC SWEEP — comments/docstrings that described deleted machinery as live:
dispatch() (allow-list + a deleted _log_allow_list_once), queue_backend/__init__
("observable but inert"), pg_queue/__init__, pg_queue/README.md's routing table,
both executor_rpc adapter headers ("Zero-regression ... delegates to Celery"),
PgExecutionDispatcher.dispatch's own headers paragraph, the orphaned
PG_QUEUE_FLAG_KEY comment now heading EXECUTE_TASK, sample.env, and an
operator-facing RuntimeError in pg_barrier naming the removed flag.

Deleted: backend/pg_queue/flags.py (zero importers), the gutted webhook_send()
(returned None from a -> str), and TestFairnessHeaders (pinned the wire shape of a
helper whose value was inert on the only transport that runs). Fixed
sample.compose.override.yaml building worker-pg-metrics from backend.Dockerfile,
which rejects `pg-queue-consumer` and crash-loops.

DELIBERATELY NOT FIXED — DEFAULT_WORKFLOW_TRANSPORT stays CELERY.
Flipping it to PG_QUEUE is the right fix for the review's other Critical and is
one line, but it breaks 19 tests across 6 files, every one relying on the implicit
Celery default — most characterising PgBarrier's Celery-link branch. Verified by
running it both ways. Rewriting those to state their transport explicitly is a
behaviour change that deserves its own review, not a ride-along in a doc sweep.
The hazard is now documented in full at the constant, with the blast radius named.

Verified: OSS backend 1005 passed, workers 1498 passed, pre-commit clean.
(An earlier run showing 6 failed/108 errors was three suites racing one Postgres —
"test_unstract_db does not exist" — not a regression; re-run serially it is green.)

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@muhammad-ali-e

Copy link
Copy Markdown
Contributor Author

Worked through the review findings — a501ae754.

Critical: fixed and mutation-verified

PgExecutionDispatcher.dispatch() takes no headers; three live call sites passed it, including the main structure_pipeline path. Reproduced the TypeError, then removed the argument and the now-dead _fairness_headers(). Nothing is lost — the router never forwarded headers to the PG path (its own docstring said so), and PG carries org routing in the enqueue payload.

The reason it shipped green was 12 bare MagicMock() dispatcher doubles. MagicMock(spec=...) does not fix that — I tried it first and the mutation stayed green, because spec= restricts attribute names, not signatures. Switched to create_autospec; re-adding headers= now turns 11 tests red.

Highs: fixed

  • compose.debug.yaml had 15 celery -A worker invocations and zero PG consumers — renamed keys, unchanged bodies. Rewritten to wrap python -m pg_queue_consumer, with the queue set deliberately not restated (the module reads it from the base file; restating it is what let this drift). Merged debug stack now has zero Celery workers.
  • PG_SCHEDULER_ADOPT_PERIODICS was set nowhere; added to the backend service.
  • run-worker.sh was missing pg-notification and pg-ide-callback, plus three drifted queue lists. Synced against docker compose config — 23 queues, exact parity both ways.
  • entrypoint.sh / ownership.py told an SRE a restart moves nothing. Corrected.

Plus deletions (flags.py, the gutted webhook_send() returning None from a -> str, TestFairnessHeaders), the sample.compose.override.yaml build fix, and ~15 docstrings/comments that described deleted machinery as live.

One High deliberately NOT fixed, and why

DEFAULT_WORKFLOW_TRANSPORT stays CELERY. Flipping it is the right fix and it is one line — but I ran it, and it breaks 19 tests across 6 files, every one relying on the implicit Celery default, most characterising PgBarrier's Celery-link branch. Rewriting those to state their transport explicitly is a behaviour change that deserves its own review; bundling it into a doc sweep is how regressions ship, and it is the pattern this review criticised elsewhere. The hazard, the mechanism and the exact blast radius are now documented at the constant.

Verified

Backend 1005 passed, workers 1498 passed, pre-commit clean. One honest note: an earlier run reported 6 failed / 108 errors — that was three suites racing a single Postgres (test_unstract_db does not exist), not a regression. Re-run serially it is green.

SonarCloud reported 5 MAJOR code smells on this PR, all python:S1172
"Remove the unused function parameter". They are the accepted-and-ignored
parameters the standardized review also flagged, so both analyses agree; fixed
rather than suppressed.

- select_backend(task_name) -> select_backend(). The answer stopped depending on
  the task name when the allow-list went; the parameter made that non-obvious.
  Removing it makes it structural.
- resolve_schedule_owner(pipeline_id, organization_id) -> resolve_schedule_owner().
  Both were unused. Worse, the arity promised a PER-SCHEDULE decision that the
  body cannot make — it is one process-global env read, so two pipelines in the
  same process can never resolve differently. 2 production + 4 test call sites.
- get_executor_dispatcher(celery_app) -> get_executor_dispatcher(), in both the
  backend and workers adapters.

CORRECTION on that last one. The docstring said the parameter was kept because
"~20 call sites across both repos" would otherwise need editing, and the review I
posted contradicted it with "there are four production call sites" — relayed from
an agent without verification. Counted properly: 15 production call sites (2 OSS,
13 cloud). The docstring was approximately right and my review comment was wrong.
Updated all 15 anyway: an ignored parameter is decoration rather than a contract,
and reading as "this dispatcher may use Celery" is exactly the belief that left a
headers= argument at three call sites and broke every extraction.

Two follow-on test repairs, both caught by running the suites:
- A regex removing `get_executor_dispatcher(celery_app=X)` also matched the
  DEFINITION line of a test fake, leaving its body referencing an undefined name.
  Both spies now record the call rather than the app, which is the property that
  survives (UN-3779: the factory must be used, not a raw ExecutionDispatcher).
- Two tests still passed a name to select_backend().

Verified: OSS backend 1005 passed, workers 1498 passed, merged tree 2005 passed,
ruff clean on every changed file.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@muhammad-ali-e
muhammad-ali-e marked this pull request as ready for review August 31, 2026 11:52
@greptile-apps

greptile-apps Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR removes the PostgreSQL queue rollout flag and makes PostgreSQL the default execution and scheduling transport.

  • Replaces flag-based transport selection with direct PostgreSQL queue dispatch.
  • Enables PostgreSQL schedule ownership by default.
  • Reconfigures the local Compose stack around PostgreSQL consumers and removes obsolete Celery workers and endpoints.
  • Retains transport fields required by worker payload contracts while removing dead rollout plumbing.

Confidence Score: 5/5

The PR appears safe to merge because no blocking failure eligible for this follow-up review remains.

No blocking failure remains.

Important Files Changed

Filename Overview
backend/workflow_manager/workflow_v2/workflow_helper.py Makes PostgreSQL queue dispatch unconditional while preserving dispatch bookkeeping and the worker transport payload.
workers/queue_backend/routing.py Removes rollout allow-list selection and makes PostgreSQL the default backend while retaining explicit backend overrides.
backend/scheduler/ownership.py Removes per-schedule rollout decisions and makes PostgreSQL ownership depend only on the scheduler enablement setting.
docker/docker-compose.yaml Promotes PostgreSQL queue consumers into the default stack and removes the replaced Celery worker services.
backend/notification_v2/notification_dispatch.py Simplifies buffered webhook delivery to an unconditional PostgreSQL enqueue while retaining terminal-failure semantics.
backend/prompt_studio/prompt_studio_core_v2/views.py Uses PostgreSQL task results unconditionally for Prompt Studio task-status polling.
unstract/workflow-execution/src/unstract/workflow_execution/executor_rpc.py Removes routing-dispatcher fallback behavior so executor request-reply operations use PostgreSQL directly.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[Backend producers] --> B[PostgreSQL queue]
    B --> C[Role-specific PG consumers]
    C --> D[Registered worker tasks]
    D --> E[Workflow and notification state]
    F[Backend startup] --> G[Schedule ownership convergence]
    G --> H[PostgreSQL scheduler]
    H --> B
Loading

Reviews (2): Last reviewed commit: "UN-4046 [FIX] e2e: retarget the test com..." | Re-trigger Greptile

…umers

CI e2e could not bring the stack up at all:

  service "worker-file-processing-v2" has neither an image nor a build context
  specified: invalid compose project

tests/compose/docker-compose.test.yaml still overrode three Celery services this
PR deleted from docker/docker-compose.yaml — worker-executor-v2,
worker-file-processing-v2 and worker-api-deployment-v2. An override for a service
that does not exist is NOT a no-op: compose synthesises it with neither image nor
build context and rejects the WHOLE project, so `docker compose up` failed before
a single test ran. I missed this file when removing the eleven Celery services.

Retargeted onto the PG consumers that replaced them, preserving each override's
purpose:
  worker-executor-v2       -> worker-pg-executor          (LLM mock: the execute
  worker-file-processing-v2-> worker-pg-fileproc           path must never reach
                                                           a real provider)
  worker-api-deployment-v2 -> worker-pg-orchestrator-api  (MAX_PARALLEL_FILE_BATCHES
                                                           fallback, kept in step so
                                                           it cannot silently
                                                           serialise the fan-out test)

Verified: `docker compose -f docker/docker-compose.yaml -f
tests/compose/docker-compose.test.yaml config` now exits 0 (it exited 1 before).
tests/README.md's reference to the old service name updated with it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@sonarqubecloud

sonarqubecloud Bot commented Sep 1, 2026

Copy link
Copy Markdown

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Unstract test results

Per-group results

Status Group Tier Passed Failed Errors Skipped Duration (s)
e2e-api-deployment e2e 3 0 0 0 25.1
e2e-coowners e2e 1 0 0 0 1.4
e2e-etl e2e 1 0 0 0 10.7
e2e-login e2e 2 0 0 0 1.5
e2e-prompt-studio e2e 1 0 0 0 4.5
e2e-smoke e2e 2 0 0 0 1.3
e2e-workflow e2e 1 0 0 0 20.6
integration-backend integration 310 0 0 26 47.2
integration-connectors integration 1 0 0 7 8.0
integration-workers integration 157 0 0 1 50.5
unit-backend unit 1124 0 0 1 41.4
unit-connectors unit 63 0 0 0 10.0
unit-core unit 33 0 0 0 1.4
unit-platform-service unit 15 0 0 0 2.6
unit-rig unit 117 0 0 0 5.3
unit-runner unit 5 0 0 0 2.9
unit-sdk1 unit 563 0 0 0 28.8
unit-workers unit 1365 0 0 1 123.1
TOTAL 3764 0 0 36 386.4

Critical paths

⚠️ Critical paths not yet covered

  • workflow-execution-fan-out — Multi-file workflow execution fans out to file-processing workers and rejoins. (declared coverage: no groups declared)
✅ Covered critical paths
  • auth-login — covered by e2e-login
  • adapter-register-llm — covered by integration-backend
  • workflow-author — covered by integration-backend
  • co-owner-manage — covered by integration-backend, e2e-coowners
  • workflow-create-execute — covered by e2e-workflow
  • api-deployment-provision — covered by integration-backend
  • api-deployment-auth — covered by integration-backend
  • api-deployment-run — covered by e2e-api-deployment
  • mcp-server-auth — covered by integration-backend
  • mcp-platform-auth — covered by integration-backend
  • prompt-studio-author — covered by integration-backend
  • prompt-studio-fetch-response — covered by e2e-prompt-studio
  • connector-register-test — covered by integration-backend
  • pipeline-etl-execute — covered by e2e-etl
  • usage-aggregate-read — covered by integration-backend
  • usage-token-tracking — covered by e2e-api-deployment
  • callback-result-delivery — covered by e2e-api-deployment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant