Feature/trial clone foundation#1434
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_step_org_and_user created User + OrgUser + UserPreferences but never UserAttributes, unlike every canonical signup path. Downstream login/activation reads UserAttributes.email_verified, so trial admins would break there. Create it alongside OrgUser.
The .pgc dump created for the template→trial data copy was never removed, leaving a full copy of the template warehouse's data at rest on local disk and leaking disk space on every clone. Remove it in a finally block covering both the success and failure paths; the manifest still records the path that was used.
_step_warehouse carried the template Postgres destination's full config forward via dict(template_creds), so a tunnel_method pointing at the template's bastion survived into the trial destination — which points at the trials-RDS host with no such tunnel, producing a misconfigured Airbyte destination. Pop tunnel_method and its dependent keys; ssl_mode and schema are left untouched since same-instance measurement keeps them valid. Chose to omit the key entirely (vs. setting NO_TUNNEL) because the rest of the codebase already treats tunnel_method as optional — both dbtfunctions.map_airbyte_destination_spec_to_dbtcli_profile and utils/helpers.py gate on `if "tunnel_method" in conn_info`, and create_warehouse forwards airbyteConfig to Airbyte's create_destination without requiring the key.
…e (A2) clone_template_org marked a failed run FAILED and re-raised, but left the created Org, its Airbyte workspace, and any provisioned trials-RDS database behind. Add a best-effort teardown before re-raising: - reuse OrgCleanupService(org, dry_run=False).delete_org() (not hand-rolled) when a trial_org was created - add warehouse_provision.drop_trial_database(), mirroring provision_trial_database, and call it when the warehouse step got far enough to record trial_warehouse_db in the manifest - the whole teardown is wrapped in its own try/except that logs failures but never masks the original exception, which still propagates and the run still ends up FAILED
…tion trialclone.trial_org and manifest["trial_warehouse_db"] were being set as the LAST statement of their respective step functions, after the org and RDS database already existed. A failure later in the same step left the external resource created but the teardown marker unset, so the best-effort cleanup in clone_template_org silently skipped it — a real resource leak. Move both assignments (with immediate save) to run right after the resource is actually created, so mid-step failures still trigger OrgCleanupService / drop_trial_database on the way out. Also make UserAttributes creation idempotent (get_or_create) since the admin User is fetched via get_or_create and can be a pre-existing user for a recurring trial_email.
Replace trial_{id} naming (which reused admin credentials) with
deterministic ft_<email-key>_db / ft_<email-key>_user names, each trial
getting its own owner role instead of sharing the RDS master account.
provision_trial_database/drop_trial_database now take the trial email.
_step_warehouse and the failure-path teardown in clone_template_org now call provision_trial_database/drop_trial_database with trial_email instead of trialclone.id, matching the new per-email db/role scheme. Also records manifest["trial_warehouse_role"] alongside the existing trial_warehouse_db teardown marker.
Reject Try Now for emails that already have a Dalgo account (real customer or prior trial), before any TrialClone row or provisioning happens, so the frontend can later route these users to login instead of cloning.
|
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:
WalkthroughThis PR adds public free-trial signup, activation, retry, status, asynchronous progress reporting, and an eight-step template organization clone covering PostgreSQL, Airbyte, dbt, Prefect, visualizations, timing, cleanup, and trial email notifications. ChangesTrial workspace flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant TrialClient
participant TrialAPI
participant Redis
participant Celery
participant CloneService
participant ExternalResources
TrialClient->>TrialAPI: submit and activate trial
TrialAPI->>Redis: consume token and acquire clone lock
TrialAPI->>Celery: enqueue clone_trial_org_task
Celery->>CloneService: run eight-step clone
CloneService->>ExternalResources: provision and clone trial resources
Celery->>Redis: write progress and completion status
TrialClient->>TrialAPI: poll clone status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ddpui/core/trial/clone_service.py`:
- Around line 29-35: Update account_exists_for_email to normalize the incoming
email once using the existing _ft_key() canonicalization, then perform a
case-insensitive username lookup so legacy accounts with differing casing are
also detected. Preserve the boolean existence result and avoid re-normalizing
the email in multiple places.
- Around line 65-75: Validate and load the admin_role in the trial clone flow
before calling User.objects.get_or_create. Move the ACCOUNT_MANAGER_ROLE lookup
and missing-role RuntimeError ahead of user creation, while preserving the
existing role validation behavior and subsequent user setup.
- Around line 186-196: Update the clone admission flow surrounding
account_exists_for_email() and TrialClone.objects.create() to serialize
concurrent requests by canonical email using an appropriate database constraint
or lock. Ensure only one request can reserve/admit a given canonical trial
email, with the losing request raising TrialAccountExistsError before
organization creation proceeds; do not rely solely on the existing pre-check.
- Around line 212-220: Update the failed-clone teardown around
OrgCleanupService.delete_org and drop_trial_database to use independent guarded
cleanup blocks, ensuring database cleanup is attempted even when organization
deletion raises while preserving the original clone error. Adjust
test_clone_teardown_failure_does_not_mask_original_error to expect both cleanup
attempts rather than skipped database teardown.
In `@ddpui/core/trial/warehouse_provision.py`:
- Around line 61-80: Update the provisioning function containing the
_admin_connect, ft_db_conn, and schema-grant operations to catch any failure
after CREATE DATABASE, best-effort invoke drop_trial_database(email), then
re-raise the original exception. Ensure cleanup failures do not replace or
suppress the original provisioning error, while preserving normal connection
closing behavior.
- Around line 36-44: Add the same 15-second connect_timeout used by the existing
warehouse client to the psycopg2.connect call in _admin_connect, ensuring trial
provisioning does not block indefinitely when the trials RDS is unavailable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 546a3d5a-ec1d-4ccf-af14-b98261d8424b
📒 Files selected for processing (17)
ddpui/core/trial/__init__.pyddpui/core/trial/clone_service.pyddpui/core/trial/exceptions.pyddpui/core/trial/timing.pyddpui/core/trial/warehouse_data.pyddpui/core/trial/warehouse_provision.pyddpui/management/commands/clone_template_org.pyddpui/migrations/0167_trialclone.pyddpui/models/__init__.pyddpui/models/trial_clone.pyddpui/settings.pyddpui/tests/core/trial/__init__.pyddpui/tests/core/trial/test_clone_service.pyddpui/tests/core/trial/test_timing.pyddpui/tests/core/trial/test_warehouse_data.pyddpui/tests/core/trial/test_warehouse_provision.pyddpui/tests/helper/test_trial_clone_model.py
| def account_exists_for_email(email: str) -> bool: | ||
| """True if a Dalgo User already exists for this email (real customer OR prior trial). | ||
|
|
||
| Dalgo creates users with username == email, so this is the account-existence check the | ||
| Try Now entry flow uses to route existing users to the login screen instead of cloning. | ||
| """ | ||
| return User.objects.filter(username=email).exists() |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Canonicalize email identity before checking account existence.
User.username is checked exactly, but _ft_key() lowercases emails. Case variants can therefore create distinct users while targeting the same PostgreSQL database and role. Normalize once at entry and use a case-insensitive lookup for legacy accounts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ddpui/core/trial/clone_service.py` around lines 29 - 35, Update
account_exists_for_email to normalize the incoming email once using the existing
_ft_key() canonicalization, then perform a case-insensitive username lookup so
legacy accounts with differing casing are also detected. Preserve the boolean
existence result and avoid re-normalizing the email in multiple places.
| if account_exists_for_email(trial_email): | ||
| raise TrialAccountExistsError( | ||
| f"an account already exists for {trial_email}; direct the user to log in" | ||
| ) | ||
|
|
||
| template = Org.objects.get(id=template_org_id) | ||
| trialclone = TrialClone.objects.create( | ||
| template_org=template, | ||
| trial_email=trial_email, | ||
| status=TrialCloneStatus.RUNNING.value, | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make clone admission atomic per canonical email.
Concurrent requests can both pass account_exists_for_email() and begin creating organizations before colliding on the deterministic database name. Serialize admission with a database constraint/lock rather than relying on this TOCTOU check.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ddpui/core/trial/clone_service.py` around lines 186 - 196, Update the clone
admission flow surrounding account_exists_for_email() and
TrialClone.objects.create() to serialize concurrent requests by canonical email
using an appropriate database constraint or lock. Ensure only one request can
reserve/admit a given canonical trial email, with the losing request raising
TrialAccountExistsError before organization creation proceeds; do not rely
solely on the existing pre-check.
| def _admin_connect(dbname: str): | ||
| """Connect to the trials-RDS instance as master, autocommit (DDL can't run in a txn).""" | ||
| conn = psycopg2.connect( | ||
| host=settings.TRIALS_RDS_HOST, | ||
| port=settings.TRIALS_RDS_PORT, | ||
| user=settings.TRIALS_RDS_ADMIN_USER, | ||
| password=settings.TRIALS_RDS_ADMIN_PASSWORD, | ||
| dbname=dbname, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'connect_timeout|psycopg2\.connect\s*\(' ddpui
fd -a '^(requirements.*|pyproject\.toml|Pipfile|poetry\.lock)$' . \
-x rg -n 'psycopg2'Repository: DalgoT4D/DDP_backend
Length of output: 5532
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== warehouse_provision.py =="
sed -n '1,140p' ddpui/core/trial/warehouse_provision.py | cat -n
echo
echo "== connect_timeout references =="
rg -n -C2 'connect_timeout|TRIALS_RDS_.*TIMEOUT|psycopg2\.connect\(' ddpui/core ddpui/utils ddpui/tests
echo
echo "== settings definitions =="
rg -n -C2 'TRIALS_RDS_(HOST|PORT|ADMIN_USER|ADMIN_PASSWORD|.*TIMEOUT|CONNECT_TIMEOUT)|connect_timeout' ddpuiRepository: DalgoT4D/DDP_backend
Length of output: 16832
Add connect_timeout to this PostgreSQL connection too.
The existing warehouse client already uses a 15s timeout; this path should do the same so RDS outages don’t block trial provisioning indefinitely.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ddpui/core/trial/warehouse_provision.py` around lines 36 - 44, Add the same
15-second connect_timeout used by the existing warehouse client to the
psycopg2.connect call in _admin_connect, ensuring trial provisioning does not
block indefinitely when the trials RDS is unavailable.
Drop the TrialClone Django model (+ migration + model tests). Per-run state (timings, manifest, trial_org) now lives in an in-memory CloneRun dataclass threaded through clone_service and step_timer, since the table added no value the product needs (timings go to stdout, reap uses OrgPlans+age, teardown uses deterministic names). Preserves all existing behavior: account-exists guard fires before any resource is created, teardown ordering fixes (trial_org/manifest markers set immediately after the resource exists), tunnel-config stripping, dump-file cleanup in finally, and no-mask teardown on failure. makemigrations --check is clean; no TrialClone references remain. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Trial org name derived from sha8(email) instead of random uuid4, so the org name is deterministic/idempotent per email like the ft_ warehouse db/role names. Adds shared email_hash8 helper reused by _ft_key. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_step_connections was selecting every stream in the freshly-discovered trial catalog, causing trial connections to over-sync the source's whole schema even when the template connection curated only a subset. Now the template's selected stream names are extracted from its syncCatalog and used to filter the discovered streams, still forcing full_refresh/overwrite for first-sync safety. Falls back to selecting all discovered streams if the template exposes no selection info, so a clone never ends up with zero streams. Also corrects the _teardown comment: sources/connections are reaped by OrgCleanupService.delete_org() via explicit per-OrgTask/per-source deletion in delete_warehouse()/delete_airbyte_workspace(), not purely by workspace-delete cascade. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copies OrgDbtModel/OrgDbtOperation/DbtEdge and CanvasNode/CanvasEdge rows from the template OrgDbt onto a freshly-created trial OrgDbt, remapping FKs via an old->new model_map. sql_path is left None (regenerated in a later step); uuid is always freshly minted (OrgDbtOperation.uuid is a NOT NULL unique field with no model default, so it must be supplied explicitly, matching existing codebase convention).
_step_dbt sets up a fresh managed dbt workspace via setup_managed_git_workspace (new GitHub repo + empty OrgDbt scaffold + cli block), then calls copy_dbt_dag to deep-copy the template's transform DAG onto it. Records dbt_repo/dbt_models in run.manifest. Wired into clone_template_org after connections as step6_dbt. .sql regeneration + git push are deferred to a later task — the trial dbt project is left as an empty scaffold with copied DB rows only.
…ions copy_dbt_dag copied OrgDbtOperation.config verbatim, leaving seq==1 operations' input_models[*].uuid pointing at the TEMPLATE org's OrgDbtModel rows even though the trial models get fresh uuids. Build a uuid_map alongside model_map and remap input_models uuids via a deep-copied config so the template's config is never mutated. Also turn a bare KeyError into a clear RuntimeError when a canvas node references a dbtmodel outside the template dbt.
Materializes sources.yml for copied SOURCE models and regenerates .sql for copied MODEL models (topological order per DbtEdge, config built from the copied CanvasNode operation chain via tranverse_graph_and_return_operations_list + create_or_update_dbt_model_in_project_v2, mirroring transform_api.py:post_terminate_operation_node), then commits+pushes to the trial's managed GitHub repo via GitManager + the org-admin PAT.
_step_dbt now regenerates every copied model's .sql + sources.yml and pushes to the trial's managed repo right after copying the DAG rows, so the clone's dbt project is no longer left as an empty scaffold. Records run.manifest["dbt_regenerated"].
…minal Adds a 2-model dependency-chain test asserting regeneration happens in topological order, and a test proving a MODEL canvas node with two incoming CanvasEdges is unsafe today (regen proceeds down an arbitrary edge instead of failing loud). Also strengthens the GitManager construction assertion to check the actual repo_dir/PAT args instead of just call count. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ddpui/core/trial/warehouse_provision.py (1)
148-151: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocstring contradicts the new retry/terminate logic.
"Requires no active sessions on template_db (it must be a frozen template)"is now stale:_create_database_from_templateactively terminates other sessions ontemplate_dband retries onObjectInUserather than requiring the caller to guarantee a frozen template. Worth wording this as "handled automatically" to avoid future readers thinking they must manually ensure zero connections.📝 Suggested docstring fix
- empty database. Requires no active sessions on template_db (it must be a frozen template). + empty database. Any other sessions on template_db are terminated before the copy (see + `_create_database_from_template`); the caller does not need to pre-freeze it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ddpui/core/trial/warehouse_provision.py` around lines 148 - 151, Update the docstring for _create_database_from_template so it describes that active sessions on template_db are handled automatically by terminating them and retrying on ObjectInUse, rather than requiring callers to provide a frozen template with no sessions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ddpui/core/trial/viz_clone.py`:
- Around line 203-206: Update the dashboard clone construction in the visible
clone flow to copy the source dashboard’s published_at value when preserving
is_published, instead of allowing it to reset to None. Keep unpublished clones’
publication metadata unset, and use the source dashboard object already
referenced as d.
---
Outside diff comments:
In `@ddpui/core/trial/warehouse_provision.py`:
- Around line 148-151: Update the docstring for _create_database_from_template
so it describes that active sessions on template_db are handled automatically by
terminating them and retrying on ObjectInUse, rather than requiring callers to
provide a frozen template with no sessions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40d45efe-481a-47d2-bfba-f5fb5e0a64d6
📒 Files selected for processing (15)
ddpui/api/trial_api.pyddpui/core/trial/clone_service.pyddpui/core/trial/dbt_clone.pyddpui/core/trial/prefect_clone.pyddpui/core/trial/tasks.pyddpui/core/trial/timing.pyddpui/core/trial/viz_clone.pyddpui/core/trial/warehouse_provision.pyddpui/tests/core/trial/test_clone_service.pyddpui/tests/core/trial/test_prefect_clone.pyddpui/tests/core/trial/test_source_config.pyddpui/tests/core/trial/test_timing.pyddpui/tests/core/trial/test_trial_task.pyddpui/tests/core/trial/test_viz_clone.pyddpui/tests/core/trial/test_warehouse_provision.py
💤 Files with no reviewable changes (2)
- ddpui/tests/core/trial/test_source_config.py
- ddpui/api/trial_api.py
🚧 Files skipped from review as they are similar to previous changes (7)
- ddpui/tests/core/trial/test_timing.py
- ddpui/core/trial/prefect_clone.py
- ddpui/tests/core/trial/test_prefect_clone.py
- ddpui/core/trial/tasks.py
- ddpui/tests/core/trial/test_trial_task.py
- ddpui/core/trial/dbt_clone.py
- ddpui/tests/core/trial/test_clone_service.py
| is_public=is_public, | ||
| public_share_token=(secrets.token_urlsafe(48) if is_public else None), | ||
| public_shared_at=(timezone.now() if is_public else None), | ||
| is_org_default=d.is_org_default, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve published_at for published dashboards.
The clone retains is_published but silently resets published_at to None. This loses publication metadata and can make a published trial dashboard inconsistent with its template.
Proposed fix
filter_layout=d.filter_layout,
is_published=d.is_published,
+ published_at=d.published_at,
is_public=is_public,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| is_public=is_public, | |
| public_share_token=(secrets.token_urlsafe(48) if is_public else None), | |
| public_shared_at=(timezone.now() if is_public else None), | |
| is_org_default=d.is_org_default, | |
| published_at=d.published_at, | |
| is_public=is_public, | |
| public_share_token=(secrets.token_urlsafe(48) if is_public else None), | |
| public_shared_at=(timezone.now() if is_public else None), | |
| is_org_default=d.is_org_default, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ddpui/core/trial/viz_clone.py` around lines 203 - 206, Update the dashboard
clone construction in the visible clone flow to copy the source dashboard’s
published_at value when preserving is_published, instead of allowing it to reset
to None. Keep unpublished clones’ publication metadata unset, and use the source
dashboard object already referenced as d.
Summary by CodeRabbit
New Features
Bug Fixes
Tests