utils/migrate-to-enterprise: initial commit #444

Merged

ryansquared merged 2 commits from ryansquared/migrate-to-enterprise.sh into main

2026-09-08 01:55:22 +00:00

ryansquared commented

2026-08-24 22:41:51 +00:00

(Migrated from codeberg.org)

No description provided.

caution-bot commented

2026-08-25 01:48:05 +00:00

(Migrated from codeberg.org)

Review findings — changes requested

I found several correctness blockers in utils/migrate-to-enterprise.sh:

Critical — the SQL is never sent to PostgreSQL (:74). docker exec omits -i, so the heredoc is not attached to stdin. psql exits successfully and the script prints Done. without validating or changing anything. A direct probe showed docker exec ... psql produced no output, while docker exec -i ... psql executed the query.
Critical — adding -i still leaves invalid SQL (:77-79). psql does not interpolate :'org_id' inside the dollar-quoted DO $$ ... $$ body. With stdin attached, PostgreSQL receives the colon literally and reports syntax error at or near ":". Pass the value outside the block (for example via set_config/current_setting) or restructure the SQL.
High — the advertised row-count/cardinality safeguards are absent (:82,113-137). affected_rows is unused, LIMIT 1 masks multiple open ledger segments, the ledger row is not locked, and neither update checks that exactly one row changed. A concurrent rollover can therefore leave an open legacy ledger segment after conversion. Lock/revalidate the ledger state and raise unless each expected update affects exactly one row.
High — conversion races the legacy tier-change path (src/api/src/subscriptions.rs:886-1073). Tier change reads billing_source before opening its transaction. If it reads legacy_credits before this migration commits, it can later overwrite the unlimited max_apps and insert a new open ledger segment while leaving the source as enterprise. Both paths need a shared organization-level lock or equivalent in-transaction guarded revalidation.
Medium — “active” currently means any non-canceled status (:99-104). This includes past_due, but enterprise deployment requires status exactly active (src/api/src/main.rs:2227-2233). Converting a legacy past_due row removes it from legacy maintenance while leaving an unusable enterprise entitlement. Require active or explicitly normalize the intended status under lock.

Checks run against head b6b2c2d661698b0b4fae5d86e562ca1c5b0c2af6: git diff --check, bash -n, direct Docker/stdin probes, a read-only nonexistent-UUID execution, schema inspection, and an independent review. The first four findings should be addressed before merge; a focused execution test would have caught both critical failures.

## Review findings — changes requested I found several correctness blockers in `utils/migrate-to-enterprise.sh`: 1. **Critical — the SQL is never sent to PostgreSQL (`:74`).** `docker exec` omits `-i`, so the heredoc is not attached to stdin. `psql` exits successfully and the script prints `Done.` without validating or changing anything. A direct probe showed `docker exec ... psql` produced no output, while `docker exec -i ... psql` executed the query. 2. **Critical — adding `-i` still leaves invalid SQL (`:77-79`).** `psql` does not interpolate `:'org_id'` inside the dollar-quoted `DO $$ ... $$` body. With stdin attached, PostgreSQL receives the colon literally and reports `syntax error at or near ":"`. Pass the value outside the block (for example via `set_config`/`current_setting`) or restructure the SQL. 3. **High — the advertised row-count/cardinality safeguards are absent (`:82,113-137`).** `affected_rows` is unused, `LIMIT 1` masks multiple open ledger segments, the ledger row is not locked, and neither update checks that exactly one row changed. A concurrent rollover can therefore leave an open legacy ledger segment after conversion. Lock/revalidate the ledger state and raise unless each expected update affects exactly one row. 4. **High — conversion races the legacy tier-change path (`src/api/src/subscriptions.rs:886-1073`).** Tier change reads `billing_source` before opening its transaction. If it reads `legacy_credits` before this migration commits, it can later overwrite the unlimited `max_apps` and insert a new open ledger segment while leaving the source as `enterprise`. Both paths need a shared organization-level lock or equivalent in-transaction guarded revalidation. 5. **Medium — “active” currently means any non-canceled status (`:99-104`).** This includes `past_due`, but enterprise deployment requires status exactly `active` (`src/api/src/main.rs:2227-2233`). Converting a legacy `past_due` row removes it from legacy maintenance while leaving an unusable enterprise entitlement. Require `active` or explicitly normalize the intended status under lock. Checks run against head `b6b2c2d661698b0b4fae5d86e562ca1c5b0c2af6`: `git diff --check`, `bash -n`, direct Docker/stdin probes, a read-only nonexistent-UUID execution, schema inspection, and an independent review. The first four findings should be addressed before merge; a focused execution test would have caught both critical failures.

caution-bot commented

2026-08-25 18:26:54 +00:00

(Migrated from codeberg.org)

Re-reviewed head 1fe8ab4158e010ccd236d3fc28baf606b3cdd945. The docker exec -i and UUID interpolation issues are fixed. Three items remain:

Enforce ledger cardinality and update counts. Removing LIMIT 1 is insufficient: non-STRICT PL/pgSQL SELECT INTO still silently picks one row. Use INTO STRICT/an explicit count, lock the open ledger row, and abort unless each expected update affects exactly one row.
Prevent the stale tier-change race. A tier change can read legacy_credits before migration, then overwrite max_apps and open a new ledger segment after migration commits. Use a shared organization-level lock or revalidate billing_source inside the tier-change transaction before writing.
Require an actually active subscription. The migration still accepts past_due/other non-canceled statuses, but enterprise deployment permits only active. Require status = 'active' or explicitly normalize the intended status under lock.

These remain blockers before merge.

Re-reviewed head `1fe8ab4158e010ccd236d3fc28baf606b3cdd945`. The `docker exec -i` and UUID interpolation issues are fixed. Three items remain: 1. **Enforce ledger cardinality and update counts.** Removing `LIMIT 1` is insufficient: non-`STRICT` PL/pgSQL `SELECT INTO` still silently picks one row. Use `INTO STRICT`/an explicit count, lock the open ledger row, and abort unless each expected update affects exactly one row. 2. **Prevent the stale tier-change race.** A tier change can read `legacy_credits` before migration, then overwrite `max_apps` and open a new ledger segment after migration commits. Use a shared organization-level lock or revalidate `billing_source` inside the tier-change transaction before writing. 3. **Require an actually active subscription.** The migration still accepts `past_due`/other non-canceled statuses, but enterprise deployment permits only `active`. Require `status = 'active'` or explicitly normalize the intended status under lock. These remain blockers before merge.

No reviewers

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

public/platform!444

No description provided.

Rows
Columns