aws client refactor and cleanup - #1262
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRemoves DBT workspace/GitHub endpoints, Celery tasks, and DBT scaffolding; deletes a CI workflow; adds a centralized AWSClient utility and replaces ad-hoc boto3 usage (SES/S3/SecretsManager); expands and reorganizes environment template and updates related tests. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(52,152,219,0.5)
participant App
end
rect rgba(46,204,113,0.5)
participant AWSClient
end
rect rgba(155,89,182,0.5)
participant SecretsManager
participant S3
participant SES
end
App->>AWSClient: request client for service (s3/secretsmanager/ses)
AWSClient->>AWSClient: check cache & per-service lock
alt client missing
AWSClient->>SecretsManager: init session/client
AWSClient->>S3: init session/client
AWSClient->>SES: init session/client
end
AWSClient-->>App: return boto3 client
App->>S3: perform S3 operation
App->>SecretsManager: get secret
App->>SES: send email
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
Ishankoradia
temporarily deployed
to
github-pages
March 9, 2026 08:12 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ddpui/tests/integration_tests/dbt_automation/test_postgres_ops.py (1)
92-96:⚠️ Potential issue | 🔴 Critical
sync_sourcesis used but not imported.The function
sync_sourcesis called on line 92 but is not imported at the top of the file. This will cause aNameErrorat runtime.Add the missing import:
from ddpui.core.dbt_automation.operations.syncsources import sync_sources🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddpui/tests/integration_tests/dbt_automation/test_postgres_ops.py` around lines 92 - 96, The call to sync_sources in the test uses a function that isn't imported, causing a NameError; add the missing import for sync_sources (from ddpui.core.dbt_automation.operations.syncsources import sync_sources) near the other test imports so TestPostgresOperations can call sync_sources successfully; ensure the import is placed with other test helpers and adjust any import grouping or linter formatting as needed..env.template (1)
160-168:⚠️ Potential issue | 🟠 MajorDon’t put docker-only bootstrap credentials in the shared template.
This block redefines
FIRST_ORG_NAME/FIRST_USER_EMAILfrom Lines 26-28 and adds default admin passwords. In a copied.env, the later entries win, so non-docker environments can silently inherit dev bootstrap credentials. Move these to a docker-specific template or leave them as commented examples only.Safer template shape
-# DOCKER DEVELOPMENT OVERRIDES -FIRST_ORG_NAME="admin-dev" -FIRST_USER_EMAIL="admin@gmail.com" -FIRST_USER_PASSWORD="password" -FIRST_USER_ROLE="Super Admin" -ADMIN_USER_EMAIL="admin@gmaio.com" -ADMIN_USER_PASSWORD="password" +# DOCKER DEVELOPMENT OVERRIDES +# Copy these into a local docker-only env file when needed. +# FIRST_ORG_NAME=admin-dev +# FIRST_USER_EMAIL=admin@example.com +# FIRST_USER_PASSWORD= +# FIRST_USER_ROLE=Super Admin +# ADMIN_USER_EMAIL=admin@example.com +# ADMIN_USER_PASSWORD=🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.template around lines 160 - 168, The .env.template currently defines docker-only bootstrap credentials (FIRST_ORG_NAME, FIRST_USER_EMAIL, FIRST_USER_PASSWORD, FIRST_USER_ROLE, ADMIN_USER_EMAIL, ADMIN_USER_PASSWORD) which can override earlier safe defaults; remove or neutralize this block from .env.template and instead place these values in a docker-specific template (e.g., .env.docker.template) or convert them to commented example lines in .env.template so non-docker environments don’t inherit dev credentials—ensure the duplicate FIRST_* and ADMIN_* variables are no longer redefined in the shared template.
🧹 Nitpick comments (2)
ddpui/utils/secretsmanager.py (1)
4-6: Consider removing unusedboto3import.After switching to
AWSClient.get_instance("secretsmanager"), the directboto3import on line 4 appears unused. TheDevSecretsManagerclass doesn't use boto3 either.♻️ Proposed fix
import os import json from uuid import uuid4 -import boto3 from ddpui.utils.custom_logger import CustomLogger from ddpui.utils.aws_client import AWSClient🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddpui/utils/secretsmanager.py` around lines 4 - 6, Remove the now-unused boto3 import: delete the top-level "import boto3" since DevSecretsManager uses AWSClient.get_instance("secretsmanager") and boto3 is not referenced anywhere in this module; ensure only needed imports remain (e.g., CustomLogger, AWSClient) and run a quick lint or import-check to confirm no other references to boto3 in this file.ddpui/tests/utils/test_aws_client.py (1)
181-198: Use distinct mocks here to prove per-service isolation.Both
boto3.Sessioncalls currently return the same mocked session/client, so this only provesSession(...)was invoked twice. It would still pass ifAWSClientaccidentally reused the first service’s client fors3. Return separate session/client pairs viaside_effectand assert the two services get different clients.Possible test tightening
- mock_boto_session = MagicMock() - mock_session.return_value = mock_boto_session - mock_client = MagicMock() - mock_boto_session.client.return_value = mock_client + sm_session = MagicMock() + s3_session = MagicMock() + sm_client_obj = MagicMock(name="secretsmanager_client") + s3_client_obj = MagicMock(name="s3_client") + sm_session.client.return_value = sm_client_obj + s3_session.client.return_value = s3_client_obj + mock_session.side_effect = [sm_session, s3_session] # Get clients for different services sm_client = AWSClient.get_instance("secretsmanager") s3_client = AWSClient.get_instance("s3") # Should create separate sessions assert mock_session.call_count == 2 + assert sm_client is sm_client_obj + assert s3_client is s3_client_obj + assert sm_client is not s3_client # Verify correct credentials were used for each session calls = mock_session.call_args_list assert calls[0][1]["aws_access_key_id"] == "test-sm-key" assert calls[1][1]["aws_access_key_id"] == "test-s3-key"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddpui/tests/utils/test_aws_client.py` around lines 181 - 198, The test test_separate_sessions_for_different_services currently uses a single MagicMock for boto3.Session so it only verifies Session was called twice, not that distinct sessions/clients were returned per service; change mock_session to return distinct session/client pairs via side_effect (e.g., two different MagicMocks) so AWSClient.get_instance("secretsmanager") and AWSClient.get_instance("s3") receive different session objects, then assert the returned clients are not the same and that mock_session.call_args_list contains the appropriate aws_access_key_id values; reference the test function test_separate_sessions_for_different_services, the mock for boto3.Session, and AWSClient.get_instance to locate and update the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.template:
- Around line 49-67: The .env template renamed AWS credential variables (e.g.,
SES_ACCESS_KEY_ID / SES_SECRET_ACCESS_KEY and S3_AWS_ACCESS_KEY_ID /
S3_AWS_SECRET_ACCESS_KEY) which breaks CI/deployments when those vars are
missing; make the change backwards-compatible by supporting both the new and old
names: update the configuration loader (where environment variables are read for
SES and S3 credentials, e.g., the code that references SES_ACCESS_KEY_ID,
SES_SECRET_ACCESS_KEY, S3_AWS_ACCESS_KEY_ID, S3_AWS_SECRET_ACCESS_KEY,
ELEMENTARY_S3_BUCKET) to prefer the new variables but fall back to the previous
variable names if the new ones are unset, or alternatively include a rollout
checklist to update all CI/deploy secrets in lockstep before merging.
In `@ddpui/tests/integration_tests/dbt_automation/test_bigquery_ops.py`:
- Around line 78-79: The test calls scaffold(...) but never imports it, causing
a NameError; update the test to either restore the missing import for scaffold
(so scaffold is available where TestBigqueryOperations.wc_client and tmpdir are
used) or replace the call with the new scaffolding API used in this repo (swap
the scaffold(TestBigqueryOperations.wc_client, tmpdir) invocation for the
current helper function or fixture and pass the same
TestBigqueryOperations.wc_client and tmpdir arguments); ensure the chosen
function is imported at top of the file so the reference to scaffold (or its
replacement) resolves.
In `@ddpui/tests/integration_tests/dbt_automation/test_postgres_ops.py`:
- Around line 79-80: The test calls scaffold (scaffold(config,
TestPostgresOperations.wc_client, tmpdir)) but the function is not imported,
causing a NameError; add the missing import for scaffold at the top of the test
file (use the same import statement used in test_bigquery_ops.py) so scaffold is
available when TestPostgresOperations.wc_client is passed to it.
In `@ddpui/utils/aws_client.py`:
- Around line 58-63: The lock acquisition path for
cls._locks[service_name].acquire(timeout=10) doesn't handle a False return and
can leave cls._sessions[service_name] as None leading to an AttributeError;
update the logic in the block that calls cls._locks[service_name].acquire to
either raise a clear exception when acquire(...) returns False or loop/wait
until the session is initialized, and ensure that
cls._initialize_session(service_name) is called (or confirmed completed) before
releasing the lock and before any use of cls._sessions[service_name].client;
reference the cls._locks, cls._sessions, cls._initialize_session, acquire, and
service_name symbols when making the change.
- Around line 109-114: The loop over cls.SUPPORTED_SERVICES currently uses a
bare except when calling cls._locks[service_name].release(); change the broad
except to catch only RuntimeError (the error raised when releasing an unlocked
lock) so you do not swallow KeyboardInterrupt/SystemExit or other unexpected
exceptions—update the try/except around the release call in the method that
references cls._locks and cls.SUPPORTED_SERVICES to use "except RuntimeError"
and keep the pass.
- Around line 57-69: The client creation has a race: after releasing
cls._locks[service_name] multiple threads can create duplicate entries in
cls._clients. Move the block that checks and assigns cls._clients[service_name]
= cls._sessions[service_name].client(service_name) and the logger.debug call
inside the same lock protected section (around where _initialize_session is
invoked), and perform the presence check again before creating the client to
ensure only one client is constructed for the given service_name.
In `@ddpui/utils/awsses.py`:
- Around line 4-6: The module-level initialization ses =
AWSClient.get_instance("ses") in ddpui/utils/awsses.py causes import-time
failures when SES credentials are missing; change this to lazy initialization by
removing the top-level ses assignment and implementing a get_ses_client()
function (or SESClient singleton getter) that calls
AWSClient.get_instance("ses") on first use and caches the result (similar to
secretsmanager.py), so importing the module no longer requires SES env vars to
be present.
---
Outside diff comments:
In @.env.template:
- Around line 160-168: The .env.template currently defines docker-only bootstrap
credentials (FIRST_ORG_NAME, FIRST_USER_EMAIL, FIRST_USER_PASSWORD,
FIRST_USER_ROLE, ADMIN_USER_EMAIL, ADMIN_USER_PASSWORD) which can override
earlier safe defaults; remove or neutralize this block from .env.template and
instead place these values in a docker-specific template (e.g.,
.env.docker.template) or convert them to commented example lines in
.env.template so non-docker environments don’t inherit dev credentials—ensure
the duplicate FIRST_* and ADMIN_* variables are no longer redefined in the
shared template.
In `@ddpui/tests/integration_tests/dbt_automation/test_postgres_ops.py`:
- Around line 92-96: The call to sync_sources in the test uses a function that
isn't imported, causing a NameError; add the missing import for sync_sources
(from ddpui.core.dbt_automation.operations.syncsources import sync_sources) near
the other test imports so TestPostgresOperations can call sync_sources
successfully; ensure the import is placed with other test helpers and adjust any
import grouping or linter formatting as needed.
---
Nitpick comments:
In `@ddpui/tests/utils/test_aws_client.py`:
- Around line 181-198: The test test_separate_sessions_for_different_services
currently uses a single MagicMock for boto3.Session so it only verifies Session
was called twice, not that distinct sessions/clients were returned per service;
change mock_session to return distinct session/client pairs via side_effect
(e.g., two different MagicMocks) so AWSClient.get_instance("secretsmanager") and
AWSClient.get_instance("s3") receive different session objects, then assert the
returned clients are not the same and that mock_session.call_args_list contains
the appropriate aws_access_key_id values; reference the test function
test_separate_sessions_for_different_services, the mock for boto3.Session, and
AWSClient.get_instance to locate and update the test.
In `@ddpui/utils/secretsmanager.py`:
- Around line 4-6: Remove the now-unused boto3 import: delete the top-level
"import boto3" since DevSecretsManager uses
AWSClient.get_instance("secretsmanager") and boto3 is not referenced anywhere in
this module; ensure only needed imports remain (e.g., CustomLogger, AWSClient)
and run a quick lint or import-check to confirm no other references to boto3 in
this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69991bc8-9441-451e-8bce-c25ef5f8363e
📒 Files selected for processing (17)
.env.template.github/workflows/dbt-automation-ui4t-ci.ymlddpui/api/dbt_api.pyddpui/api/transform_api.pyddpui/celeryworkers/tasks.pyddpui/core/dbt_automation/operations/scaffold.pyddpui/ddpdbt/elementary_service.pyddpui/ddpprefect/schema.pyddpui/tests/api_tests/test_dbt_api.pyddpui/tests/core/test_celery_tasks.pyddpui/tests/integration_tests/dbt_automation/test_bigquery_ops.pyddpui/tests/integration_tests/dbt_automation/test_postgres_ops.pyddpui/tests/utils/test_aws_client.pyddpui/utils/aws_client.pyddpui/utils/awsses.pyddpui/utils/secretsmanager.pydocs/docs/contributing.md
💤 Files with no reviewable changes (7)
- .github/workflows/dbt-automation-ui4t-ci.yml
- ddpui/ddpprefect/schema.py
- ddpui/tests/api_tests/test_dbt_api.py
- ddpui/core/dbt_automation/operations/scaffold.py
- ddpui/api/dbt_api.py
- ddpui/celeryworkers/tasks.py
- ddpui/api/transform_api.py
Ishankoradia
temporarily deployed
to
github-pages
March 9, 2026 08:26 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Ishankoradia
temporarily deployed
to
github-pages
March 9, 2026 08:43 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1262 +/- ##
==========================================
+ Coverage 55.06% 55.34% +0.28%
==========================================
Files 124 124
Lines 14716 14668 -48
==========================================
+ Hits 8103 8118 +15
+ Misses 6613 6550 -63 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Ishankoradia
temporarily deployed
to
github-pages
March 10, 2026 09:29 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ddpui/tests/services/test_elementary_service.py (1)
386-410:⚠️ Potential issue | 🟡 MinorAvoid leaking
LATEST_ELEMENTARY_PACKAGE_VERSIONinto later tests.This mutates process-wide env state and only cleans it up at the bottom. If the assertion fails, later tests inherit the override;
delalso drops any pre-existing value instead of restoring it.🧪 Safer test-scoped env override
- # Set environment variable to control the expected upgrade version - os.environ["LATEST_ELEMENTARY_PACKAGE_VERSION"] = "0.20.0" - - response = check_dbt_files(org) + # Set environment variable to control the expected upgrade version + with patch.dict(os.environ, {"LATEST_ELEMENTARY_PACKAGE_VERSION": "0.20.0"}): + response = check_dbt_files(org) mock_gather_dbt_project_params.assert_called_once_with(org, org.dbt) assert response == ( None, @@ }, }, ) - - # Clean up environment variable - del os.environ["LATEST_ELEMENTARY_PACKAGE_VERSION"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddpui/tests/services/test_elementary_service.py` around lines 386 - 410, The test mutates process-wide LATEST_ELEMENTARY_PACKAGE_VERSION and deletes it at the end which can leak state; instead, save the original value (orig = os.environ.get("LATEST_ELEMENTARY_PACKAGE_VERSION")), set os.environ["LATEST_ELEMENTARY_PACKAGE_VERSION"] = "0.20.0", run the call to check_dbt_files(org) and assertions (including mock_gather_dbt_project_params), and then in a finally block restore the environment: if orig is None remove the key else set it back to orig; alternatively use the test fixture monkeypatch.setenv/monkeypatch.delenv to scope the change to the test.
♻️ Duplicate comments (1)
ddpui/utils/aws_client.py (1)
55-61:⚠️ Potential issue | 🟠 MajorRe-check the cache after taking the service lock.
Two threads can both pass Line 55 before the first client is stored. The second thread then acquires the lock later and constructs another boto3 client, so the per-service singleton guarantee still is not enforced.
🔒 Serialize creation with an inner cache check
`@classmethod` def _get_client(cls, service_name: str): """Get client for the specified service""" - if service_name not in cls._clients or cls._clients[service_name] is None: - if cls._locks[service_name].acquire(timeout=10): - try: - boto_session = cls._initialize_boto_session(service_name) - cls._clients[service_name] = boto_session.client(service_name) - finally: - cls._locks[service_name].release() - else: - raise RuntimeError( - f"Timeout while acquiring lock for {service_name} session initialization" - ) - - if cls._clients[service_name] is None: + client = cls._clients.get(service_name) + if client is None: + if not cls._locks[service_name].acquire(timeout=10): + raise RuntimeError( + f"Timeout while acquiring lock for {service_name} session initialization" + ) + try: + client = cls._clients.get(service_name) + if client is None: + boto_session = cls._initialize_boto_session(service_name) + client = boto_session.client(service_name) + cls._clients[service_name] = client + finally: + cls._locks[service_name].release() + + if client is None: raise RuntimeError(f"Failed to initialize client for {service_name}") - return cls._clients[service_name] + return clientRun this to confirm the current implementation still has only the outer
_clientsguard and no second guard inside the locked section:#!/bin/bash python - <<'PY' import ast from pathlib import Path path = Path("ddpui/utils/aws_client.py") src = path.read_text() tree = ast.parse(src) for node in tree.body: if isinstance(node, ast.ClassDef) and node.name == "AWSClient": for fn in node.body: if isinstance(fn, ast.FunctionDef) and fn.name == "_get_client": print(ast.get_source_segment(src, fn)) print("\n_guards mentioning _clients:") for inner in ast.walk(fn): if isinstance(inner, ast.If): cond = ast.get_source_segment(src, inner.test) or "" if "_clients" in cond: print(f"Line {inner.lineno}: {cond}") PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddpui/utils/aws_client.py` around lines 55 - 61, The _get_client method in class AWSClient currently checks cls._clients before acquiring cls._locks[service_name] but does not re-check inside the locked section, allowing two threads to create duplicate clients; modify _get_client so that after acquiring cls._locks[service_name] you immediately re-check if cls._clients[service_name] is still None (or missing) and only then call cls._initialize_boto_session and assign cls._clients[service_name] = boto_session.client(service_name); keep the existing try/finally that releases the lock and ensure the inner check references the same cls._clients and service_name symbols to enforce the per-service singleton.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ddpui/utils/aws_client.py`:
- Around line 77-96: The error message for missing AWS credentials uses
service_name.upper() to build env var names but the code actually reads
different env vars (e.g., S3_AWS_ACCESS_KEY_ID / S3_AWS_SECRET_ACCESS_KEY for
the "s3" branch); update the ValueError so it lists the exact env var names the
code reads for each branch (use S3_AWS_ACCESS_KEY_ID and
S3_AWS_SECRET_ACCESS_KEY for service_name == "s3", SECRETSMANAGER_ACCESS_KEY_ID
and SECRETSMANAGER_SECRET_ACCESS_KEY for "secretsmanager", and SES_ACCESS_KEY_ID
and SES_SECRET_ACCESS_KEY for "ses") or alternatively change the env var lookups
to match the diagnostic names — adjust the missing-credentials check around the
service_name branch where access_key/secret_key and service_display are set.
- Around line 108-116: The reset_instance method should not forcibly release
locks held by other threads; instead iterate cls.SUPPORTED_SERVICES and for each
lock in cls._locks acquire() it and then release() in a finally block to ensure
you only release locks you own and avoid the RuntimeError and silent failures;
update reset_instance to remove the locked() check and bare except, and use a
try: cls._locks[service_name].acquire() followed by a finally:
cls._locks[service_name].release() (or use context management if you wrap the
lock) so the lock lifecycle is correct relative to _get_client and other
callers.
---
Outside diff comments:
In `@ddpui/tests/services/test_elementary_service.py`:
- Around line 386-410: The test mutates process-wide
LATEST_ELEMENTARY_PACKAGE_VERSION and deletes it at the end which can leak
state; instead, save the original value (orig =
os.environ.get("LATEST_ELEMENTARY_PACKAGE_VERSION")), set
os.environ["LATEST_ELEMENTARY_PACKAGE_VERSION"] = "0.20.0", run the call to
check_dbt_files(org) and assertions (including mock_gather_dbt_project_params),
and then in a finally block restore the environment: if orig is None remove the
key else set it back to orig; alternatively use the test fixture
monkeypatch.setenv/monkeypatch.delenv to scope the change to the test.
---
Duplicate comments:
In `@ddpui/utils/aws_client.py`:
- Around line 55-61: The _get_client method in class AWSClient currently checks
cls._clients before acquiring cls._locks[service_name] but does not re-check
inside the locked section, allowing two threads to create duplicate clients;
modify _get_client so that after acquiring cls._locks[service_name] you
immediately re-check if cls._clients[service_name] is still None (or missing)
and only then call cls._initialize_boto_session and assign
cls._clients[service_name] = boto_session.client(service_name); keep the
existing try/finally that releases the lock and ensure the inner check
references the same cls._clients and service_name symbols to enforce the
per-service singleton.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38fc92bb-2c5b-4b70-b4b6-d0b6b9dcb75b
📒 Files selected for processing (4)
ddpui/tests/api_tests/test_dbt_api.pyddpui/tests/api_tests/test_transform_api.pyddpui/tests/services/test_elementary_service.pyddpui/utils/aws_client.py
Comment on lines
+77
to
+96
| # Get credentials based on service | ||
| if service_name == "secretsmanager": | ||
| access_key = os.getenv("SECRETSMANAGER_ACCESS_KEY_ID") | ||
| secret_key = os.getenv("SECRETSMANAGER_SECRET_ACCESS_KEY") | ||
| service_display = "Secrets Manager" | ||
| elif service_name == "s3": | ||
| access_key = os.getenv("S3_AWS_ACCESS_KEY_ID") | ||
| secret_key = os.getenv("S3_AWS_SECRET_ACCESS_KEY") | ||
| service_display = "S3" | ||
| elif service_name == "ses": | ||
| access_key = os.getenv("SES_ACCESS_KEY_ID") | ||
| secret_key = os.getenv("SES_SECRET_ACCESS_KEY") | ||
| service_display = "SES" | ||
| else: | ||
| raise ValueError(f"Unsupported service: {service_name}") | ||
|
|
||
| if not access_key or not secret_key: | ||
| raise ValueError( | ||
| f"Missing {service_display} AWS credentials: {service_name.upper()}_ACCESS_KEY_ID and {service_name.upper()}_SECRET_ACCESS_KEY required" | ||
| ) |
There was a problem hiding this comment.
S3's missing-credentials error points to the wrong env vars.
The S3 branch reads S3_AWS_ACCESS_KEY_ID / S3_AWS_SECRET_ACCESS_KEY, but the ValueError message tells operators to set S3_ACCESS_KEY_ID / S3_SECRET_ACCESS_KEY. That makes misconfiguration harder to diagnose.
🛠️ Keep the env names alongside the values
# Get credentials based on service
if service_name == "secretsmanager":
- access_key = os.getenv("SECRETSMANAGER_ACCESS_KEY_ID")
- secret_key = os.getenv("SECRETSMANAGER_SECRET_ACCESS_KEY")
+ access_key_env = "SECRETSMANAGER_ACCESS_KEY_ID"
+ secret_key_env = "SECRETSMANAGER_SECRET_ACCESS_KEY"
service_display = "Secrets Manager"
elif service_name == "s3":
- access_key = os.getenv("S3_AWS_ACCESS_KEY_ID")
- secret_key = os.getenv("S3_AWS_SECRET_ACCESS_KEY")
+ access_key_env = "S3_AWS_ACCESS_KEY_ID"
+ secret_key_env = "S3_AWS_SECRET_ACCESS_KEY"
service_display = "S3"
elif service_name == "ses":
- access_key = os.getenv("SES_ACCESS_KEY_ID")
- secret_key = os.getenv("SES_SECRET_ACCESS_KEY")
+ access_key_env = "SES_ACCESS_KEY_ID"
+ secret_key_env = "SES_SECRET_ACCESS_KEY"
service_display = "SES"
else:
raise ValueError(f"Unsupported service: {service_name}")
+ access_key = os.getenv(access_key_env)
+ secret_key = os.getenv(secret_key_env)
if not access_key or not secret_key:
raise ValueError(
- f"Missing {service_display} AWS credentials: {service_name.upper()}_ACCESS_KEY_ID and {service_name.upper()}_SECRET_ACCESS_KEY required"
+ f"Missing {service_display} AWS credentials: {access_key_env} and {secret_key_env} required"
)🧰 Tools
🪛 Ruff (0.15.5)
[warning] 91-91: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 94-96: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ddpui/utils/aws_client.py` around lines 77 - 96, The error message for
missing AWS credentials uses service_name.upper() to build env var names but the
code actually reads different env vars (e.g., S3_AWS_ACCESS_KEY_ID /
S3_AWS_SECRET_ACCESS_KEY for the "s3" branch); update the ValueError so it lists
the exact env var names the code reads for each branch (use S3_AWS_ACCESS_KEY_ID
and S3_AWS_SECRET_ACCESS_KEY for service_name == "s3",
SECRETSMANAGER_ACCESS_KEY_ID and SECRETSMANAGER_SECRET_ACCESS_KEY for
"secretsmanager", and SES_ACCESS_KEY_ID and SES_SECRET_ACCESS_KEY for "ses") or
alternatively change the env var lookups to match the diagnostic names — adjust
the missing-credentials check around the service_name branch where
access_key/secret_key and service_display are set.
Comment on lines
+108
to
+116
| def reset_instance(cls): | ||
| """Reset all sessions and clients""" | ||
| # Release any locks that might be held | ||
| for service_name in cls.SUPPORTED_SERVICES: | ||
| try: | ||
| if cls._locks[service_name].locked(): | ||
| cls._locks[service_name].release() | ||
| except: | ||
| pass |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import threading
import time
lock = threading.Lock()
def holder():
lock.acquire()
print("holder acquired lock")
time.sleep(0.2)
try:
lock.release()
print("holder released lock")
except RuntimeError as exc:
print(f"holder release failed: {type(exc).__name__}: {exc}")
def external_reset():
time.sleep(0.05)
if lock.locked():
lock.release()
print("other thread released holder's lock")
t1 = threading.Thread(target=holder)
t2 = threading.Thread(target=external_reset)
t1.start()
t2.start()
t1.join()
t2.join()
PYRepository: DalgoT4D/DDP_backend
Length of output: 179
🏁 Script executed:
cat -n ddpui/utils/aws_client.py | sed -n '100,130p'Repository: DalgoT4D/DDP_backend
Length of output: 784
🏁 Script executed:
cat -n ddpui/utils/aws_client.py | head -80Repository: DalgoT4D/DDP_backend
Length of output: 3427
🏁 Script executed:
rg "reset_instance" ddpui/Repository: DalgoT4D/DDP_backend
Length of output: 327
🏁 Script executed:
cat -n ddpui/tests/utils/test_aws_client.pyRepository: DalgoT4D/DDP_backend
Length of output: 8595
reset_instance() must not release locks it did not acquire.
Lines 111–116 forcibly release any locked threading.Lock. If another thread is inside _get_client() (lines 56–61), releasing its lock from outside breaks the critical section, allows a second initializer entry, and causes the original finally: release() to raise RuntimeError. The bare except on line 115 silences this error, leaving the lock in an inconsistent state.
Replace the pattern by acquiring and properly releasing all locks:
Fix: Reset by acquiring the locks, not by breaking them
`@classmethod`
def reset_instance(cls):
"""Reset all sessions and clients"""
- # Release any locks that might be held
- for service_name in cls.SUPPORTED_SERVICES:
- try:
- if cls._locks[service_name].locked():
- cls._locks[service_name].release()
- except:
- pass
-
- cls._clients = {}
+ acquired_locks = []
+ try:
+ for service_name in sorted(cls.SUPPORTED_SERVICES):
+ lock = cls._locks[service_name]
+ if not lock.acquire(timeout=10):
+ raise RuntimeError(
+ f"Timeout while acquiring lock for {service_name} reset"
+ )
+ acquired_locks.append(lock)
+
+ cls._clients = {}
+ finally:
+ for lock in reversed(acquired_locks):
+ lock.release()🧰 Tools
🪛 Ruff (0.15.5)
[error] 115-115: Do not use bare except
(E722)
[error] 115-116: try-except-pass detected, consider logging the exception
(S110)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ddpui/utils/aws_client.py` around lines 108 - 116, The reset_instance method
should not forcibly release locks held by other threads; instead iterate
cls.SUPPORTED_SERVICES and for each lock in cls._locks acquire() it and then
release() in a finally block to ensure you only release locks you own and avoid
the RuntimeError and silent failures; update reset_instance to remove the
locked() check and bare except, and use a try:
cls._locks[service_name].acquire() followed by a finally:
cls._locks[service_name].release() (or use context management if you wrap the
lock) so the lock lifecycle is correct relative to _get_client and other
callers.
Ishankoradia
temporarily deployed
to
github-pages
March 11, 2026 06:00 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ddpui/utils/aws_client.py`:
- Around line 52-71: The _get_client method currently acquires
cls._locks[service_name] then performs a double-check and only enters a
try/finally when creating the client, which leaks the lock if the double-check
shows the client already exists; fix by moving the try/finally to immediately
after a successful acquire so the lock is always released: after if
cls._locks[service_name].acquire(timeout=10): enter a try block (and finally
release the lock) and inside the try do the double-check and call
cls._initialize_boto_session and boto_session.client(service_name) only when
needed to set cls._clients[service_name]; keep the outer existence checks on
cls._clients and the RuntimeError on timeout unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7f44dfa-13ec-4850-81d5-7da350aa8e69
📒 Files selected for processing (1)
ddpui/utils/aws_client.py
Comment on lines
+52
to
+71
| @classmethod | ||
| def _get_client(cls, service_name: str): | ||
| """Get client for the specified service""" | ||
| if service_name not in cls._clients or cls._clients[service_name] is None: | ||
| if cls._locks[service_name].acquire(timeout=10): | ||
| if service_name not in cls._clients or cls._clients[service_name] is None: | ||
| try: | ||
| boto_session = cls._initialize_boto_session(service_name) | ||
| cls._clients[service_name] = boto_session.client(service_name) | ||
| finally: | ||
| cls._locks[service_name].release() | ||
| else: | ||
| raise RuntimeError( | ||
| f"Timeout while acquiring lock for {service_name} session initialization" | ||
| ) | ||
|
|
||
| if cls._clients[service_name] is None: | ||
| raise RuntimeError(f"Failed to initialize client for {service_name}") | ||
|
|
||
| return cls._clients[service_name] |
There was a problem hiding this comment.
Lock is never released when client already exists after acquiring lock.
When acquire(timeout=10) succeeds at line 56 but the double-check at line 57 finds a client already created by another thread, the code skips the try block entirely and the lock is never released. This causes a permanent lock leak.
The finally block at lines 61-62 only executes when the try block at line 58 is entered.
🔒 Proposed fix to ensure lock is always released
`@classmethod`
def _get_client(cls, service_name: str):
"""Get client for the specified service"""
if service_name not in cls._clients or cls._clients[service_name] is None:
if cls._locks[service_name].acquire(timeout=10):
- if service_name not in cls._clients or cls._clients[service_name] is None:
- try:
+ try:
+ if service_name not in cls._clients or cls._clients[service_name] is None:
boto_session = cls._initialize_boto_session(service_name)
cls._clients[service_name] = boto_session.client(service_name)
- finally:
- cls._locks[service_name].release()
+ finally:
+ cls._locks[service_name].release()
else:
raise RuntimeError(
f"Timeout while acquiring lock for {service_name} session initialization"
)🧰 Tools
🪛 Ruff (0.15.5)
[warning] 64-66: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 69-69: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ddpui/utils/aws_client.py` around lines 52 - 71, The _get_client method
currently acquires cls._locks[service_name] then performs a double-check and
only enters a try/finally when creating the client, which leaks the lock if the
double-check shows the client already exists; fix by moving the try/finally to
immediately after a successful acquire so the lock is always released: after if
cls._locks[service_name].acquire(timeout=10): enter a try block (and finally
release the lock) and inside the try do the double-check and call
cls._initialize_boto_session and boto_session.client(service_name) only when
needed to set cls._clients[service_name]; keep the outer existence checks on
cls._clients and the RuntimeError on timeout unchanged.
Ishankoradia
temporarily deployed
to
github-pages
March 11, 2026 06:13 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
himanshudube97
approved these changes
Mar 11, 2026
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
2 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.
Summary by CodeRabbit
New Features
Refactor
Tests
Chores
Documentation