[FIX] Optimize queries made by worker and retry config of worker base client - #1798
Merged
Merged
Conversation
Contributor
Summary by CodeRabbit
WalkthroughSeparated single-object retrieval from list logic in the file-execution internal view, added caching to several internal client methods with new cache key types, and refactored HTTP retry handling to default per-request values from configuration. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Resolves 39.8s GET /internal/v1/file-execution/<uuid>/ latency by: - Removing 7 debug COUNT(*) full table scans from get_queryset() - Adding get_object() O(1) PK lookup override in ViewSet - Adding @with_cache decorators to pipeline fetch methods - Adding pipeline_data_key() to prevent cache key collisions - Fixing urllib3 retry amplification: clear status_forcelist, let app-level retries handle status codes - Use config defaults instead of hardcoded retry values Root cause: Count queries on every request + retry storm (urllib3 × app-level). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chandrasekharan-zipstack
force-pushed
the
fix/file-execution-queryset-perf
branch
from
February 19, 2026 05:53
61b439b to
0a83713
Compare
chandrasekharan-zipstack
changed the title
chandrasekharan-zipstack
requested review from
johnyrahul,
muhammad-ali-e and
ritwik-g
February 19, 2026 05:59
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/workflow_manager/file_execution/internal_views.py (1)
84-105:⚠️ Potential issue | 🟡 MinorStale comment —
get_object()no longer applies query-parameter filteringLine 89 reads:
# Don't use self.get_object() as it applies query parameter filtering. The newget_object()implementation at lines 33-49 only performs org-level filtering (no query-param filtering), so this justification is now incorrect and misleading.If the real reason for not using
self.get_object()here is to preserve the bespoke{"error": …, "detail": …}404 shape (vs DRF's standardNotFoundresponse), that should be the comment instead. Alternatively, if error-shape consistency is acceptable, thestatusandupdate_hashactions (lines 233-291) could both delegate directly toself.get_object()to eliminate the duplicated queryset-build logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/workflow_manager/file_execution/internal_views.py` around lines 84 - 105, The comment in status() claiming "Don't use self.get_object() as it applies query parameter filtering" is stale; update it to accurately state why self.get_object() is avoided (e.g., to preserve the bespoke 404 response shape with {"error", "detail"}) or refactor to use self.get_object() and unify behavior with update_hash() to remove duplicated queryset construction. Specifically, either replace the misleading comment with one referencing WorkflowFileExecution, filter_queryset_by_organization, and the custom 404 payload, or change status() (and update_hash()) to call self.get_object() and adapt the exception handling to return the existing {"error","detail"} shape if you need that custom response.workers/shared/clients/base_client.py (1)
199-238:⚠️ Potential issue | 🟡 MinorInternalAPIClient._make_request hardcodes
backoff_factor=0.5, bypassing WorkerConfig default of1.0The facade's signature at lines 1386-1395 passes hardcoded
max_retries: int = 3, backoff_factor: float = 0.5tobase_client._make_request(). Whilemax_retriesaligns with the config default,backoff_factor=0.5diverges fromWorkerConfig'sINTERNAL_API_RETRY_BACKOFF_FACTORdefault of1.0(workers/shared/infrastructure/config/worker_config.py, line 207). Callers using the facade cannot customize retry backoff through environment configuration.Apply the suggested signature change to allow
Noneto propagate config defaults:Suggested change
def _make_request( self, method: str, endpoint: str, data: dict[str, Any] | None = None, params: dict[str, Any] | None = None, timeout: int | None = None, max_retries: int | None = None, # was: int = 3 backoff_factor: float | None = None, # was: float = 0.5 organization_id: str | None = None, ) -> dict[str, Any]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/shared/clients/base_client.py` around lines 199 - 238, The facade (InternalAPIClient) is hardcoding backoff_factor=0.5 when calling base_client._make_request which prevents WorkerConfig defaults from applying; change the InternalAPIClient._make_request (and any facade method signatures that pass defaults) to use max_retries: int | None = None and backoff_factor: float | None = None so None will propagate into base_client._make_request (which already reads defaults from self.config), and remove the literal 0.5 default passed into base_client._make_request calls so the configured INTERNAL_API_RETRY_BACKOFF_FACTOR can take effect.
🧹 Nitpick comments (2)
workers/shared/api/internal_client.py (1)
436-436: Unused lambda parameters — Ruff ARG005The lambdas on lines 436, 492, and 502 all capture
selfandorganization_id(andcheck_activeon 502) without using them in the expression. Prefix unused params with_to satisfy Ruff and signal intent:♻️ Proposed cleanup
- lambda self, workflow_id, organization_id=None: str(workflow_id), # line 436 + lambda _self, workflow_id, _organization_id=None: str(workflow_id), - lambda self, pipeline_id, organization_id=None: str(pipeline_id), # line 492 + lambda _self, pipeline_id, _organization_id=None: str(pipeline_id), # line 502 — after the check_active fix is applied: - lambda self, pipeline_id, check_active=True, organization_id=None: f"{pipeline_id}:{check_active}", + lambda _self, pipeline_id, check_active=True, _organization_id=None: f"{pipeline_id}:{check_active}",Also applies to: 492-492, 502-502
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/shared/api/internal_client.py` at line 436, The three lambda expressions (e.g., "lambda self, workflow_id, organization_id=None: str(workflow_id)" and the similar lambdas at the other two locations) capture unused parameters; rename those unused parameters by prefixing them with an underscore (e.g., use _self, _organization_id, and _check_active where applicable) so Ruff ARG005 is satisfied and intent is clear while leaving the expression (str(workflow_id) etc.) unchanged.workers/shared/clients/base_client.py (1)
137-152: Session-level retry changes look correct; connection-error retry duplication exists but is not amplified as claimedClearing
status_forcelist=[]and settingread=0correctly delegates HTTP-status and read-timeout retries to_make_request(), fixing the documented amplification for 5xx/429 responses.Regarding connection errors: urllib3 will retry connection failures up to its
totalcounter limit (withconnectrequests within that budget), and_make_request()also retries onrequests.exceptions.ConnectionError. However, the arithmetic in the comment is inaccurate. Withtotal=api_retry_attemptsandconnect=api_retry_attempts, urllib3'stotalparameter is a global cap across all retry types—not a multiplier. Each retry attempt decrementstotal, so the effective behavior is sequential retries capped by thetotalcounter, not exponential multiplication.Still, connection-layer retries at both levels (urllib3 + app) create unnecessary duplication. Consider setting
connect=1to let app-level_make_request()manage connection retries exclusively (consistent with theread=0intent):♻️ Option: delegate connection retries solely to app-level
retry_strategy = Retry( total=self.config.api_retry_attempts, backoff_factor=self.config.api_retry_backoff_factor, status_forcelist=[], allowed_methods=allowed_http_methods, - connect=self.config.api_retry_attempts, + connect=1, # One attempt per session.request(); app-level handles retries read=0, respect_retry_after_header=True, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/shared/clients/base_client.py` around lines 137 - 152, The retry configuration in _setup_session duplicates connection-level retries because urllib3's Retry.total acts as a global cap and connect retries are counted against it, so having connect=self.config.api_retry_attempts plus app-level retries in _make_request() is unnecessary; update _setup_session to set connect=1 (or another minimal constant) instead of using api_retry_attempts so connection retries are effectively delegated to _make_request(), keep status_forcelist=[] and read=0 as-is, and ensure any comment describing retry behavior references both _setup_session and _make_request to clarify delegation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/workflow_manager/file_execution/internal_views.py`:
- Around line 44-48: The except block handling
WorkflowFileExecution.DoesNotExist should preserve the original exception
context by using exception chaining: change the handler to "except
WorkflowFileExecution.DoesNotExist as exc" and re-raise the NotFound using
"raise NotFound(f'No file execution found with ID {pk}') from exc" so the
original exception is attached (the surrounding logic including queryset.get()
and self.check_object_permissions stays the same).
In `@workers/shared/api/internal_client.py`:
- Around line 500-505: The cache key extractor used in the with_cache decorator
for get_pipeline_data currently only uses pipeline_id, so calls with differing
check_active values collide and can bypass the active check; update the lambda
passed to with_cache (the CacheType.PIPELINE_DATA key extractor) to incorporate
the check_active flag (e.g., include str(check_active) or its boolean value)
into the returned key alongside pipeline_id so get_pipeline_data(self,
pipeline_id, check_active=True/False, organization_id=None) produces distinct
keys for the two modes.
- Around line 434-437: The two methods get_workflow and get_workflow_definition
share the same cache decorator key (with_cache(CacheType.WORKFLOW, lambda self,
workflow_id, organization_id=None: str(workflow_id))) causing type collisions;
update one of them (preferably get_workflow_definition) to use a distinct cache
identity such as CacheType.WORKFLOW_DEFINITION or alter its key generator to
append a suffix (e.g., f"{workflow_id}:definition") so the cache keys differ and
each method stores/reads the correct return type.
---
Outside diff comments:
In `@backend/workflow_manager/file_execution/internal_views.py`:
- Around line 84-105: The comment in status() claiming "Don't use
self.get_object() as it applies query parameter filtering" is stale; update it
to accurately state why self.get_object() is avoided (e.g., to preserve the
bespoke 404 response shape with {"error", "detail"}) or refactor to use
self.get_object() and unify behavior with update_hash() to remove duplicated
queryset construction. Specifically, either replace the misleading comment with
one referencing WorkflowFileExecution, filter_queryset_by_organization, and the
custom 404 payload, or change status() (and update_hash()) to call
self.get_object() and adapt the exception handling to return the existing
{"error","detail"} shape if you need that custom response.
In `@workers/shared/clients/base_client.py`:
- Around line 199-238: The facade (InternalAPIClient) is hardcoding
backoff_factor=0.5 when calling base_client._make_request which prevents
WorkerConfig defaults from applying; change the InternalAPIClient._make_request
(and any facade method signatures that pass defaults) to use max_retries: int |
None = None and backoff_factor: float | None = None so None will propagate into
base_client._make_request (which already reads defaults from self.config), and
remove the literal 0.5 default passed into base_client._make_request calls so
the configured INTERNAL_API_RETRY_BACKOFF_FACTOR can take effect.
---
Nitpick comments:
In `@workers/shared/api/internal_client.py`:
- Line 436: The three lambda expressions (e.g., "lambda self, workflow_id,
organization_id=None: str(workflow_id)" and the similar lambdas at the other two
locations) capture unused parameters; rename those unused parameters by
prefixing them with an underscore (e.g., use _self, _organization_id, and
_check_active where applicable) so Ruff ARG005 is satisfied and intent is clear
while leaving the expression (str(workflow_id) etc.) unchanged.
In `@workers/shared/clients/base_client.py`:
- Around line 137-152: The retry configuration in _setup_session duplicates
connection-level retries because urllib3's Retry.total acts as a global cap and
connect retries are counted against it, so having
connect=self.config.api_retry_attempts plus app-level retries in _make_request()
is unnecessary; update _setup_session to set connect=1 (or another minimal
constant) instead of using api_retry_attempts so connection retries are
effectively delegated to _make_request(), keep status_forcelist=[] and read=0
as-is, and ensure any comment describing retry behavior references both
_setup_session and _make_request to clarify delegation.
- Chain exception context in get_object() (Ruff B904) - Fix cache key collision: get_workflow_definition() now uses CacheType.WORKFLOW_DEFINITION instead of CacheType.WORKFLOW to avoid type mismatch with get_workflow() sharing the same cache key - Include check_active in get_pipeline_data() cache key to prevent active-status bypass when check_active=False is cached first - Refactor status() and update_hash() to use self.get_object() instead of duplicating manual queryset + org filtering; add except APIException pass-through so NotFound propagates as 404 not 500 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/workflow_manager/file_execution/internal_views.py (1)
47-47: Ruff TRY003: long message string directly inNotFound(...)— minor style nit.Consider extracting the message to a variable:
✨ Proposed fix
- except WorkflowFileExecution.DoesNotExist as exc: - raise NotFound(f"No file execution found with ID {pk}") from exc + except WorkflowFileExecution.DoesNotExist as exc: + msg = f"No file execution found with ID {pk}" + raise NotFound(msg) from exc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/workflow_manager/file_execution/internal_views.py` at line 47, Extract the long error message string passed directly into NotFound into a named variable before raising; in internal_views.py where NotFound is raised (raise NotFound(f"No file execution found with ID {pk}") from exc), create a clear message variable (e.g., error_msg = f"No file execution found with ID {pk}") and pass that variable to NotFound to satisfy Ruff TRY003 and improve readability.workers/shared/cache/cache_keys.py (1)
28-31:pipeline_idparameter name is misleading — the caller passes a compound key
cache_decorator.pycallspipeline_data_key(cache_key_suffix)wherecache_key_suffixisf"{pipeline_id}:{check_active}"(e.g.,"abc-123:True"). The parameter is documented/typed aspipeline_id: strbut actually receives a compound string. No functional bug, but the name misrepresents the input contract.♻️ Suggested rename
- def pipeline_data_key(pipeline_id: str) -> str: - """Generate cache key for pipeline data.""" - return f"worker_cache:pipeline_data:{pipeline_id}" + def pipeline_data_key(key_suffix: str) -> str: + """Generate cache key for pipeline data (key_suffix is typically '{pipeline_id}:{check_active}').""" + return f"worker_cache:pipeline_data:{key_suffix}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/shared/cache/cache_keys.py` around lines 28 - 31, The parameter name pipeline_id on pipeline_data_key is misleading because callers pass a compound string (e.g., f"{pipeline_id}:{check_active}"); rename the parameter to something like cache_key_suffix or compound_key and update the docstring to state it accepts the pipeline id plus suffix (still str), then update any references/calls (e.g., in cache_decorator.py) if they rely on the old parameter name so function signature and usage are consistent; keep the returned key format unchanged.workers/shared/api/internal_client.py (1)
434-437: Cache collision betweenget_workflowandget_workflow_definitionis now resolved.
CacheType.WORKFLOW→worker_cache:workflow:{id}vsCacheType.WORKFLOW_DEFINITION→worker_cache:workflow_definition:{id}are distinct. This addresses the previously reported type-collision bug.The three lambdas at lines 436, 492, and 502 each have
selfandorganization_idflagged as unused by Ruff (ARG005). They are intentionally present to match the wrapped method's signature (the decorator callskey_extractor(self, *args, **kwargs)). Suppressing the warnings with_prefixes keeps the intent clear:♻️ Suppress ARG005 warnings
- `@with_cache`( - CacheType.WORKFLOW, - lambda self, workflow_id, organization_id=None: str(workflow_id), - ) + `@with_cache`( + CacheType.WORKFLOW, + lambda _self, workflow_id, _organization_id=None: str(workflow_id), + )Apply the same
_prefix pattern to the lambdas at lines 492 and 502.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/shared/api/internal_client.py` around lines 434 - 437, The cache key extractor lambdas passed to the with_cache decorator are triggering Ruff ARG005 for unused parameters; update each lambda used as the key_extractor (the one that currently reads "lambda self, workflow_id, organization_id=None: ..." for CacheType.WORKFLOW and the two similar lambdas used with CacheType.WORKFLOW_DEFINITION) to prefix unused params with underscores (e.g., use "_self" and "_organization_id" instead of "self" and "organization_id") so the signature still matches the wrapped method but Ruff no longer flags them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/workflow_manager/file_execution/internal_views.py`:
- Around line 33-49: get_object currently builds a queryset using
WorkflowFileExecution.objects.filter(id=pk) and then applies
filter_queryset_by_organization but bypasses DRF filter backends; update
get_object (in FileExecutionInternalViewSet) to pass the queryset through
self.filter_queryset(queryset) after applying filter_queryset_by_organization so
any configured filter_backends (filterset_class, ordering, etc.) are applied,
while preserving the existing DoesNotExist -> NotFound logic and the subsequent
self.check_object_permissions call.
In `@workers/shared/api/internal_client.py`:
- Around line 490-498: The cache is being poisoned because
internal_client.get_pipeline_type (wrapped by `@with_cache`) caches a success=true
fallback returned by execution_client.get_pipeline_type on any exception; change
execution_client.get_pipeline_type so it detects HTTP 404 responses and returns
the cacheable APIResponse.success_response(...) only for 404 (legitimate "no
pipeline-type" cases with source="fallback"), while for other exceptions
(network errors, 5xx, timeouts) it should raise/propagate the exception or
return a non-success APIResponse so the with_cache decorator will not store the
transient fallback; ensure the 404-only success behavior is done in the
execution_client.get_pipeline_type implementation and keep the cache decorator
logic unchanged.
---
Nitpick comments:
In `@backend/workflow_manager/file_execution/internal_views.py`:
- Line 47: Extract the long error message string passed directly into NotFound
into a named variable before raising; in internal_views.py where NotFound is
raised (raise NotFound(f"No file execution found with ID {pk}") from exc),
create a clear message variable (e.g., error_msg = f"No file execution found
with ID {pk}") and pass that variable to NotFound to satisfy Ruff TRY003 and
improve readability.
In `@workers/shared/api/internal_client.py`:
- Around line 434-437: The cache key extractor lambdas passed to the with_cache
decorator are triggering Ruff ARG005 for unused parameters; update each lambda
used as the key_extractor (the one that currently reads "lambda self,
workflow_id, organization_id=None: ..." for CacheType.WORKFLOW and the two
similar lambdas used with CacheType.WORKFLOW_DEFINITION) to prefix unused params
with underscores (e.g., use "_self" and "_organization_id" instead of "self" and
"organization_id") so the signature still matches the wrapped method but Ruff no
longer flags them.
In `@workers/shared/cache/cache_keys.py`:
- Around line 28-31: The parameter name pipeline_id on pipeline_data_key is
misleading because callers pass a compound string (e.g.,
f"{pipeline_id}:{check_active}"); rename the parameter to something like
cache_key_suffix or compound_key and update the docstring to state it accepts
the pipeline id plus suffix (still str), then update any references/calls (e.g.,
in cache_decorator.py) if they rely on the old parameter name so function
signature and usage are consistent; keep the returned key format unchanged.
|
Contributor
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
chandrasekharan-zipstack
deleted the
fix/file-execution-queryset-perf
branch
February 19, 2026 11:16
hari-kuriakose
pushed a commit
that referenced
this pull request
Feb 24, 2026
… client (#1798) * Fix production queryset performance and retry amplification Resolves 39.8s GET /internal/v1/file-execution/<uuid>/ latency by: - Removing 7 debug COUNT(*) full table scans from get_queryset() - Adding get_object() O(1) PK lookup override in ViewSet - Adding @with_cache decorators to pipeline fetch methods - Adding pipeline_data_key() to prevent cache key collisions - Fixing urllib3 retry amplification: clear status_forcelist, let app-level retries handle status codes - Use config defaults instead of hardcoded retry values Root cause: Count queries on every request + retry storm (urllib3 × app-level). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Address PR #1798 review comments - Chain exception context in get_object() (Ruff B904) - Fix cache key collision: get_workflow_definition() now uses CacheType.WORKFLOW_DEFINITION instead of CacheType.WORKFLOW to avoid type mismatch with get_workflow() sharing the same cache key - Include check_active in get_pipeline_data() cache key to prevent active-status bypass when check_active=False is cached first - Refactor status() and update_hash() to use self.get_object() instead of duplicating manual queryset + org filtering; add except APIException pass-through so NotFound propagates as 404 not 500 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
gaya3-vijayakumar
added a commit
that referenced
this pull request
Mar 9, 2026
…1806) * refactor: Add dynamic plugin loading for enterprise components ## What - Add dynamic plugin loading support to OSS codebase - Enable enterprise components to be loaded at runtime without modifying tracked files ## Why - Enterprise code was overwriting git-tracked OSS files causing dirty git state - Need clean separation between OSS and enterprise codebases - OSS should work independently without enterprise components ## How - `unstract_migrations.py`: Uses try/except ImportError to load from `pluggable_apps.migrations_ext` - `api_hub_usage_utils.py`: Uses try/except ImportError to load from `plugins.verticals_usage` - `utils.py`: Uses try/except ImportError to load from `pluggable_apps.manual_review_v2` and `plugins.workflow_manager.workflow_v2.rule_engine` - `backend.Dockerfile`: Conditional install of `requirements.txt` if present ## Can this PR break any existing features. If yes, please list possible items. If no, please explain why. - No. The changes add optional plugin loading that gracefully falls back to default behavior when plugins are not present. Existing OSS functionality is preserved. ## Database Migrations - None ## Env Config - None ## Relevant Docs - None ## Related Issues or PRs - None ## Dependencies Versions - None ## Notes on Testing - OSS build: Verify app starts and works without enterprise plugins - Enterprise build: Verify plugins are loaded and function correctly 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * refactor: Use get_plugin() for API Hub usage utilities 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Refactor random sampling logic in utils.py Removed redundant import of random and exception handling for manual_review_v2. Signed-off-by: Hari John Kuriakose <hari@zipstack.com> * fix: Add Traefik port labels and clean up service ignore list - Add explicit loadbalancer port labels for backend (8000) and frontend (3000) services in docker-compose to ensure proper Traefik routing - Rename spawned_services to ignored_services for clarity - Extend ignored_services list to include tool-classifier, tool-text_extractor, and worker-unified services that don't need environment setup 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: Update frontend Docker config for nginx serving Update Traefik port label to 80 to match nginx and fix Dockerfile to use BUILD_CONTEXT_PATH for the runtime config script in both dev and prod stages. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: Use ARG instead of ENV for BUILD_CONTEXT_PATH in frontend Dockerfile Convert BUILD_CONTEXT_PATH from environment variable to build argument for proper Docker multi-stage build support. ARGs must be declared globally and re-declared in each stage that needs them. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * feat: Add HubSpot integration plugin for contact event tracking - Add new integrations plugin category under backend/plugins/integrations/ - Create HubSpot plugin with event-based contact updates - Track user milestone events: project creation, document upload, prompt run, tool export, and API deployment - Plugin validates is_first_for_org flag and first org member status - Remove unused hubspot_signup_api() stub from authentication_service - Update subscription_helper to use plugin pattern for form submissions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * [FIX] Fix HITL review screen showing "Never expires" despite TTL being set (#1785) Fix two interacting bugs that prevented TTL from propagating to HITL queue records: 1. WorkflowUtil.get_hitl_ttl_seconds was an OSS stub that always returned None. Now delegates to get_hitl_ttl_seconds_by_workflow via try/except import, falling back to None in OSS environments. 2. _push_to_queue_for_api_deployment never fetched TTL. Now mirrors the connector path by calling WorkflowUtil.get_hitl_ttl_seconds and passing ttl_seconds through to _create_queue_result and _enqueue_to_packet_or_regular_queue. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * refactor: Add dynamic plugin loading for enterprise components (#1736) * refactor: Add dynamic plugin loading for enterprise components - Add dynamic plugin loading support to OSS codebase - Enable enterprise components to be loaded at runtime without modifying tracked files - Enterprise code was overwriting git-tracked OSS files causing dirty git state - Need clean separation between OSS and enterprise codebases - OSS should work independently without enterprise components - `unstract_migrations.py`: Uses try/except ImportError to load from `pluggable_apps.migrations_ext` - `api_hub_usage_utils.py`: Uses try/except ImportError to load from `plugins.verticals_usage` - `utils.py`: Uses try/except ImportError to load from `pluggable_apps.manual_review_v2` and `plugins.workflow_manager.workflow_v2.rule_engine` - `backend.Dockerfile`: Conditional install of `requirements.txt` if present - No. The changes add optional plugin loading that gracefully falls back to default behavior when plugins are not present. Existing OSS functionality is preserved. - None - None - None - None - None - OSS build: Verify app starts and works without enterprise plugins - Enterprise build: Verify plugins are loaded and function correctly 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * refactor: Use get_plugin() for API Hub usage utilities 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Refactor random sampling logic in utils.py Removed redundant import of random and exception handling for manual_review_v2. Signed-off-by: Hari John Kuriakose <hari@zipstack.com> * fix: Add Traefik port labels and clean up service ignore list - Add explicit loadbalancer port labels for backend (8000) and frontend (3000) services in docker-compose to ensure proper Traefik routing - Rename spawned_services to ignored_services for clarity - Extend ignored_services list to include tool-classifier, tool-text_extractor, and worker-unified services that don't need environment setup 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: Update frontend Docker config for nginx serving Update Traefik port label to 80 to match nginx and fix Dockerfile to use BUILD_CONTEXT_PATH for the runtime config script in both dev and prod stages. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: Use ARG instead of ENV for BUILD_CONTEXT_PATH in frontend Dockerfile Convert BUILD_CONTEXT_PATH from environment variable to build argument for proper Docker multi-stage build support. ARGs must be declared globally and re-declared in each stage that needs them. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: typo in ignored services var name * fix: handle script execution via entrypoint --------- Signed-off-by: Hari John Kuriakose <hari@zipstack.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * feat: Add auth error code for forbidden emails (#1789) * feat: add auth error code and frontend error display * fix: run frontend dev server on port 80 and add signup handler - Set PORT=80 env var in frontend Dockerfile development stage - Change EXPOSE from 3000 to 80 to match production nginx - Add handleSignup function and pass to LoginForm component 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: add ARG declaration to Dockerfile stages using BUILD_CONTEXT_PATH SonarQube flagged that ARG must be declared in each Docker build stage where it is used. Added the missing ARG BUILD_CONTEXT_PATH to both development and builder stages. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com> * [MISC] Improve dev experience by adding a compose debug override (#1765) * [MISC] Improve Docker dev experience: separate debugpy, optimize memory, add V2 workers support - Move debugpy to optional compose.debug.yaml for cleaner default dev setup - Update compose.override.yaml with memory-optimized settings (1 worker, 2 threads) - Add V2 workers configuration with build definitions - Move V1 workers to optional workers-v1 profile - Use modern uv run python -Xfrozen_modules=off pattern for services - Updated README with compose.debug.yaml usage instructions Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * [MISC] Add Docker Compose version requirement and scheduler comment - Add Docker Compose 2.24.4+ requirement note for !override directive - Add comment explaining why worker-log-history-scheduler-v2 has no watch Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * [MISC] Fix debug ports table in README - Fix port order: runner=5679, platform=5680, prompt=5681 - Remove x2text-service (not in compose.debug.yaml) - Add V2 workers debug ports (5682-5688) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * [MISC] Simplify sample.compose.override.yaml Remove db image override and V1 worker command overrides from sample file. Users should reference compose.override.yaml for actual dev setup. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> * [FIX] Optimize queries made by worker and retry config of worker base client (#1798) * Fix production queryset performance and retry amplification Resolves 39.8s GET /internal/v1/file-execution/<uuid>/ latency by: - Removing 7 debug COUNT(*) full table scans from get_queryset() - Adding get_object() O(1) PK lookup override in ViewSet - Adding @with_cache decorators to pipeline fetch methods - Adding pipeline_data_key() to prevent cache key collisions - Fixing urllib3 retry amplification: clear status_forcelist, let app-level retries handle status codes - Use config defaults instead of hardcoded retry values Root cause: Count queries on every request + retry storm (urllib3 × app-level). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Address PR #1798 review comments - Chain exception context in get_object() (Ruff B904) - Fix cache key collision: get_workflow_definition() now uses CacheType.WORKFLOW_DEFINITION instead of CacheType.WORKFLOW to avoid type mismatch with get_workflow() sharing the same cache key - Include check_active in get_pipeline_data() cache key to prevent active-status bypass when check_active=False is cached first - Refactor status() and update_hash() to use self.get_object() instead of duplicating manual queryset + org filtering; add except APIException pass-through so NotFound propagates as 404 not 500 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * UN-2971 [FEAT] Pass selectedProduct to login/signup API for OAuth product scope (#1803) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Hari John Kuriakose <hari@zipstack.com> Co-authored-by: Claude <noreply@anthropic.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix: Scope HubSpot milestone count checks to current organization PromptStudioOutputManager and DocumentManager lack DefaultOrganizationManagerMixin, so .objects.count() was counting across ALL organizations. Filter through the tool FK to CustomTool (which is org-scoped) to get correct per-org counts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * [REFACTOR] Extract HubSpot notification logic into shared utility Move all _notify_hubspot_* methods from views into a shared utils/hubspot_notify.py module with a single notify_hubspot_event() function, reducing duplication across prompt_studio views and api_deployment_views. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Signed-off-by: Hari John Kuriakose <hari@zipstack.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: vishnuszipstack <117254672+vishnuszipstack@users.noreply.github.com> Co-authored-by: Chandrasekharan M <117059509+chandrasekharan-zipstack@users.noreply.github.com> Co-authored-by: vishnuszipstack <vishnu@zipstack.com> Co-authored-by: Gayathri <142381512+gaya3-zipstack@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
FileExecutionInternalViewSetqueryset performance: addget_object()override for O(1) PK lookups, remove debugCOUNT(*)calls, remove debuglist()override@with_cachedecorator toget_workflow(),get_pipeline_type(),get_pipeline_data()in the worker API clientBaseAPIClientby clearing urllib3status_forcelist— status code retries now handled exclusively by app-level_make_request()Why
GET /internal/v1/file-execution/<uuid>/took 39.8s due toget_queryset()scanning ~30K rows with a 3-table JOIN and 7xCOUNT(*)on every request — even for retrieve-by-IDget_workflow(),get_pipeline_type(),get_pipeline_data()generated redundant identical API calls during burst trafficstatus_forcelistretries (3x) multiplied with app-level retries (4x) caused a 12-retry storm per failed request, amplifying the degradationHow
internal_views.py: Addedget_object()that queries directly by PK with org filtering — bypassesget_queryset()for retrieve/update/destroy. Removed all 7 debug.count()calls and thelist()override that returned 400 on >50 resultsinternal_client.py: Added@with_cache(CacheType.WORKFLOW)toget_workflow(),@with_cache(CacheType.PIPELINE)toget_pipeline_type(),@with_cache(CacheType.PIPELINE_DATA)toget_pipeline_data()cache_decorator.py: Wired upPIPELINEandPIPELINE_DATAcache types in the key-building switch to use dedicated key generators instead of falling through tocustom_key()cache_keys.py: Addedpipeline_data_key()to avoid cache collisions between pipeline type and pipeline database_client.py: Setstatus_forcelist=[],connect=N,read=0on urllib3 Retry so only transport-level failures retry at that layer. Changedmax_retries/backoff_factordefaults from hardcoded3/0.5toNonewith config fallbackCan this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
get_queryset()is preserved — only debugCOUNT(*)calls and logging were removed. The newget_object()override uses the same org-filtering as the existing custom actions (status,update_hash). Cache additions are transparent with graceful fallback. Retry behavior is unchanged from the caller's perspective — same retry count, same status codes retried — just no longer amplified by urllib3's redundant layer.Database Migrations
Env Config
api_retry_attemptsandapi_retry_backoff_factorfromWorkerConfigare now used as defaults instead of hardcoded values.Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
GET /internal/v1/file-execution/<uuid>/completes in <100ms (was 39.8s)COUNT(*)debug logs fromFileExecutionInternalViewSetworkflow_id/pipeline_idScreenshots
N/A — backend/worker performance fix, no UI changes.
Checklist
I have read and understood the Contribution Guidelines.