From cf2683cdcca00d0de5f353e004f246df28a87566 Mon Sep 17 00:00:00 2001 From: doswalt Date: Thu, 25 Jun 2026 13:55:18 -0400 Subject: [PATCH 01/16] feature flags complete with plan for extending to experiments as well --- .../plans/precomputed-segments-experiments.md | 291 ++++++++++++++++++ .gitignore | 1 + packages/backend/CLAUDE.md | 36 +++ .../src/api/models/PrecomputedSegment.ts | 19 ++ .../api/repositories/FeatureFlagRepository.ts | 15 + .../PrecomputedSegmentRepository.ts | 21 ++ .../src/api/repositories/SegmentRepository.ts | 8 + .../src/api/services/FeatureFlagService.ts | 114 ++++--- .../api/services/PrecomputedSegmentService.ts | 182 +++++++++++ .../src/api/services/SegmentService.ts | 20 +- packages/backend/src/app.ts | 4 + .../1779500000000-precomputedSegment.ts | 25 ++ .../init/seed/backfillPrecomputedSegments.ts | 8 + packages/types/src/Experiment/enums.ts | 1 + 14 files changed, 701 insertions(+), 44 deletions(-) create mode 100644 .claude/plans/precomputed-segments-experiments.md create mode 100644 packages/backend/src/api/models/PrecomputedSegment.ts create mode 100644 packages/backend/src/api/repositories/PrecomputedSegmentRepository.ts create mode 100644 packages/backend/src/api/services/PrecomputedSegmentService.ts create mode 100644 packages/backend/src/database/migrations/1779500000000-precomputedSegment.ts create mode 100644 packages/backend/src/init/seed/backfillPrecomputedSegments.ts diff --git a/.claude/plans/precomputed-segments-experiments.md b/.claude/plans/precomputed-segments-experiments.md new file mode 100644 index 0000000000..e6a9fd8099 --- /dev/null +++ b/.claude/plans/precomputed-segments-experiments.md @@ -0,0 +1,291 @@ +# Plan: Precomputed Segment Lists for Experiments + +Port the feature-flag precomputed segment list pattern to experiments. +The feature-flag implementation is complete and stable on `wip/segments-precalculated` — this is a 1:1 parallel. + +## Background + +Experiments currently resolve segment inclusion/exclusion **on-the-fly at assignment time** via recursive `resolveSegment()` calls in `ExperimentAssignmentService`. This is the same slow path that feature flags moved away from. The goal is to precompute flat `inclusionIds[]` / `exclusionIds[]` arrays per experiment, stored in a new `precomputed_experiment_segment` table, so the assignment path becomes a fast in-memory `Set.has()` check. + +Key difference from feature flags: experiment join tables (`ExperimentSegmentInclusion`, `ExperimentSegmentExclusion`) have **no `enabled` column** — all rows are active. The `where: { enabled: true }` filter in `recomputeForFlag` does not apply here. + +## Files to create + +- `src/api/models/PrecomputedExperimentSegment.ts` — entity +- `src/api/repositories/PrecomputedExperimentSegmentRepository.ts` — repo +- `src/database/migrations/-precomputedExperimentSegment.ts` — migration +- `src/init/seed/backfillPrecomputedExperimentSegments.ts` — startup backfill + +## Files to modify + +- `src/api/services/PrecomputedSegmentService.ts` — add experiment methods +- `src/api/services/ExperimentAssignmentService.ts` — replace `resolveSegment()` with precomputed lookup +- `src/api/services/ExperimentService.ts` — add recompute triggers on segment list mutations +- `src/app.ts` — wire startup backfill + +--- + +## Step 1 — Entity + +**`src/api/models/PrecomputedExperimentSegment.ts`** + +Mirror `PrecomputedSegment` exactly, replacing the feature flag FK with an experiment FK: + +```ts +@Entity() +export class PrecomputedExperimentSegment extends BaseModel { + @PrimaryColumn('uuid') + public experimentId: string; + + @ManyToOne(() => Experiment, { onDelete: 'CASCADE' }) + @JoinColumn({ name: 'experimentId' }) + public experiment: Experiment; + + @Column('text', { array: true, default: '{}' }) + public inclusionIds: string[]; + + @Column('text', { array: true, default: '{}' }) + public exclusionIds: string[]; +} +``` + +Register the entity in `env.ts` (wherever the entities glob is — confirm it picks up `src/api/models/*.ts` automatically; if so no change needed). + +--- + +## Step 2 — Repository + +**`src/api/repositories/PrecomputedExperimentSegmentRepository.ts`** + +Mirror `PrecomputedSegmentRepository`: + +```ts +@EntityRepository(PrecomputedExperimentSegment) +export class PrecomputedExperimentSegmentRepository extends Repository { + public async upsertByExperimentId(experimentId: string, inclusionIds: string[], exclusionIds: string[]): Promise { + await this.createQueryBuilder() + .insert() + .into(PrecomputedExperimentSegment) + .values({ experimentId, inclusionIds, exclusionIds }) + .orUpdate(['inclusionIds', 'exclusionIds', 'updatedAt'], ['experimentId']) + .execute(); + } + + public async findByExperimentIds(experimentIds: string[]): Promise<(PrecomputedExperimentSegment | null)[]> { + if (!experimentIds.length) return []; + const rows = await this.createQueryBuilder('ps') + .where('ps.experimentId IN (:...ids)', { ids: experimentIds }) + .getMany(); + return experimentIds.map((id) => rows.find((r) => r.experimentId === id) ?? null); + } +} +``` + +--- + +## Step 3 — Migration + +Generate via: +```bash +npm run migration:generate -- -n precomputedExperimentSegment +``` + +Expected output — creates `precomputed_experiment_segment` table with `experimentId` PK (uuid), `inclusionIds` text[], `exclusionIds` text[], standard `BaseModel` timestamp columns, FK to `experiment` with `ON DELETE CASCADE`. + +Verify the generated migration matches intent before running. + +--- + +## Step 4 — Service methods + +**`src/api/services/PrecomputedSegmentService.ts`** — inject `PrecomputedExperimentSegmentRepository` and `ExperimentSegmentInclusionRepository` / `ExperimentSegmentExclusionRepository` and `ExperimentRepository`, then add: + +```ts +public async recomputeForExperiment(experimentId: string, logger: UpgradeLogger): Promise { + const [inclusionRecords, exclusionRecords] = await Promise.all([ + this.experimentSegmentInclusionRepository.find({ + where: { experiment: { id: experimentId } }, + relations: ['segment'], + }), + this.experimentSegmentExclusionRepository.find({ + where: { experiment: { id: experimentId } }, + relations: ['segment'], + }), + ]); + + const inclusionSegmentIds = inclusionRecords.map((r) => r.segment.id); + const exclusionSegmentIds = exclusionRecords.map((r) => r.segment.id); + + const [inclusionIds, exclusionIds] = await Promise.all([ + this.flattenSegmentMembers(inclusionSegmentIds, new Set()), + this.flattenSegmentMembers(exclusionSegmentIds, new Set()), + ]); + + await this.precomputedExperimentSegmentRepository.upsertByExperimentId( + experimentId, + [...new Set(inclusionIds)], + [...new Set(exclusionIds)] + ); + + await this.cacheService.delCache(CACHE_PREFIX.PRECOMPUTED_EXPERIMENT_SEGMENT_KEY_PREFIX + experimentId); + logger.info({ message: `Recomputed precomputed_experiment_segment for experiment ${experimentId}` }); +} + +public scheduleRecomputeForExperimentSegment(segmentId: string, logger: UpgradeLogger): void { + this.collectAffectedExperimentIds(segmentId, new Set()) + .then((experimentIds) => Promise.all([...experimentIds].map((id) => this.recomputeForExperiment(id, logger)))) + .catch((err) => logger.error({ message: `Error in scheduleRecomputeForExperimentSegment: ${err}` })); +} + +public async getAffectedExperimentIds(segmentId: string): Promise { + return [...(await this.collectAffectedExperimentIds(segmentId, new Set()))]; +} + +public async getExperimentPrecomputedSets(experimentIds: string[]): Promise> { + if (!experimentIds.length) return new Map(); + const results = await this.cacheService.wrapFunction( + CACHE_PREFIX.PRECOMPUTED_EXPERIMENT_SEGMENT_KEY_PREFIX, + experimentIds, + () => this.precomputedExperimentSegmentRepository.findByExperimentIds(experimentIds) + ); + const map = new Map(); + experimentIds.forEach((id, i) => { + if (results[i]) map.set(id, results[i] as PrecomputedExperimentSegment); + }); + return map; +} + +public async backfillMissingExperiments(logger: UpgradeLogger): Promise { + const [allExperiments, existingRows] = await Promise.all([ + this.experimentRepository.find({ select: ['id'] }), + this.precomputedExperimentSegmentRepository.find({ select: ['experimentId'] }), + ]); + const existingIds = new Set(existingRows.map((r) => r.experimentId)); + const missing = allExperiments.filter((e) => !existingIds.has(e.id)); + if (!missing.length) { + logger.info({ message: 'precomputed_experiment_segment backfill: all experiments already have rows, nothing to do' }); + return; + } + for (const exp of missing) { + try { + await this.recomputeForExperiment(exp.id, logger); + } catch (err) { + logger.error({ message: `Failed to backfill precomputed_experiment_segment for experiment ${exp.id}: ${err}` }); + } + } + logger.info({ message: `precomputed_experiment_segment backfill complete: computed ${missing.length} of ${allExperiments.length} experiments` }); +} + +private async collectAffectedExperimentIds(segmentId: string, visited: Set): Promise> { + if (visited.has(segmentId)) return new Set(); + visited.add(segmentId); + + const [inclusionRecords, exclusionRecords] = await Promise.all([ + this.experimentSegmentInclusionRepository.find({ + where: { segment: { id: segmentId } }, + relations: ['experiment'], + }), + this.experimentSegmentExclusionRepository.find({ + where: { segment: { id: segmentId } }, + relations: ['experiment'], + }), + ]); + + const experimentIds = new Set([ + ...inclusionRecords.map((r) => r.experiment.id), + ...exclusionRecords.map((r) => r.experiment.id), + ]); + + const parentIds = await this.segmentRepository.findParentSegmentIds(segmentId); + await Promise.all( + parentIds.map(async (parentId) => { + const parentExpIds = await this.collectAffectedExperimentIds(parentId, visited); + parentExpIds.forEach((id) => experimentIds.add(id)); + }) + ); + + return experimentIds; +} +``` + +Note: `flattenSegmentMembers` is shared — no duplication needed. Only the join table queries and repository differ. + +**Add `PRECOMPUTED_EXPERIMENT_SEGMENT_KEY_PREFIX` to `CACHE_PREFIX` in `packages/types`.** + +--- + +## Step 5 — ExperimentAssignmentService read path + +This is the largest change. Currently `getIncludedAndExcludedExperiments()` → `resolveSegmentsForEntities()` → `resolveSegment()` do recursive DB queries. Replace that with a precomputed lookup. + +The pattern to follow is `FeatureFlagService.featureFlagLevelInclusionExclusion()` — call `getExperimentPrecomputedSets(experimentIds)` and replace the `includeData` / `excludeData` maps that `inclusionExclusionLogic()` currently receives from recursive resolution with maps built from the precomputed flat arrays. + +Specific methods to audit and update in `ExperimentAssignmentService.ts`: +- `getSegmentObject()` (line ~2102) — currently extracts segment IDs to resolve +- `resolveSegmentsForEntities()` (line ~2140) — drives resolution; replace with precomputed map build +- `getIncludedAndExcludedExperiments()` (line ~2156) — wires the above together +- `inclusionExclusionLogic()` (line ~2231) — the actual include/exclude evaluation; this should be largely untouched if the input maps have the same shape + +The key question to verify before implementing: does `inclusionExclusionLogic()` expect `{users: userId[], groups: {type, groupId}[]}` shaped data from resolved segments, or does it work with flat ID arrays? Flat inclusion/exclusion arrays may need a small shim to match the expected shape. Inspect the method signature and callers carefully before changing the data shape. + +--- + +## Step 6 — ExperimentService triggers + +Find the equivalents of the three `FeatureFlagService` trigger points and add matching calls. Look for methods that: +- Add a segment to an experiment's inclusion/exclusion list → `recomputeForExperiment` after +- Remove a segment from an experiment's inclusion/exclusion list → `recomputeForExperiment` after +- Update segment members in an experiment context → `recomputeForExperiment` after +- Delete an experiment's segment entirely → collect IDs before, recompute after (same pattern as `SegmentService.deleteSegment`) + +Also check if `ExperimentService` has a `deleteExperiment` path — if so, the FK cascade handles cleanup (same as feature flags), no extra work needed. + +The `SegmentService` triggers (`scheduleRecomputeForExperimentSegment`) should be added alongside the existing `scheduleRecomputeForSegment` calls at lines 440, 468, and 1027 — both feature flags and experiments need recomputing when shared segment members change. + +--- + +## Step 7 — Startup backfill + +**`src/init/seed/backfillPrecomputedExperimentSegments.ts`**: + +```ts +import { PrecomputedSegmentService } from '../../api/services/PrecomputedSegmentService'; +import { UpgradeLogger } from '../../lib/logger/UpgradeLogger'; +import Container from 'typedi'; + +export async function backfillPrecomputedExperimentSegments(logger: UpgradeLogger): Promise { + const precomputedSegmentService = Container.get(PrecomputedSegmentService); + await precomputedSegmentService.backfillMissingExperiments(logger); +} +``` + +**`src/app.ts`** — add after the existing `backfillPrecomputedSegments` call: + +```ts +.then(() => { + return backfillPrecomputedExperimentSegments(logger); +}); +``` + +--- + +## Step 8 — Update CLAUDE.md + +Add a matching section to `packages/backend/CLAUDE.md` documenting the experiment precomputed segment pattern (mirror the feature flag section already there). + +--- + +## Checklist + +- [ ] Step 1: `PrecomputedExperimentSegment` entity +- [ ] Step 2: `PrecomputedExperimentSegmentRepository` +- [ ] Step 3: Migration generated and verified +- [ ] Step 4: `PrecomputedSegmentService` experiment methods + `CACHE_PREFIX` constant +- [ ] Step 5: `ExperimentAssignmentService` read path refactored +- [ ] Step 6: `ExperimentService` write triggers added +- [ ] Step 6b: `SegmentService` triggers extended for experiments (lines 440, 468, 1027) +- [ ] Step 7: Startup backfill wired into `app.ts` +- [ ] Step 8: CLAUDE.md updated +- [ ] Typecheck passes +- [ ] Migration runs cleanly +- [ ] Manual smoke test: pre-existing experiment with segment lists shows correct assignment after restart diff --git a/.gitignore b/.gitignore index f8beb6339e..822027b4c5 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,7 @@ .DS_Store .dccache node_modules +.claude* .vscode setup_upgrade.sh .idea diff --git a/packages/backend/CLAUDE.md b/packages/backend/CLAUDE.md index 6ba35867cc..1ce4231dab 100644 --- a/packages/backend/CLAUDE.md +++ b/packages/backend/CLAUDE.md @@ -74,3 +74,39 @@ Available at `/swagger` when `SWAGGER_ENABLED=true`. Auto-generated from JSDoc ` ## Test Coverage Target ~49% current, 80% goal. Tests live in `test/` (not alongside source files). + +## Precomputed Segment Lists (Feature Flags) + +Segment inclusion/exclusion for **feature flags** is precomputed and stored flat in the `precomputed_segment` table rather than resolved on-the-fly at assignment time. + +### How it works + +- **`PrecomputedSegment` entity** (`src/api/models/PrecomputedSegment.ts`) — one row per feature flag, columns: `featureFlagId` (PK), `inclusionIds: text[]`, `exclusionIds: text[]`. FK to `feature_flag` with `onDelete: CASCADE`. +- **`PrecomputedSegmentService`** (`src/api/services/PrecomputedSegmentService.ts`) — owns all computation and cache logic: + - `recomputeForFlag(flagId)` — flattens all enabled inclusion/exclusion segments (recursive sub-segments) into flat ID arrays and upserts the row. + - `scheduleRecomputeForSegment(segmentId)` — fire-and-forget; finds all flags referencing a segment (and its parents) and calls `recomputeForFlag` for each. + - `getAffectedFlagIds(segmentId)` — public helper that returns flag IDs affected by a given segment (used before deletion). + - `getPrecomputedSets(flagIds[])` — cache-wrapped batch fetch, returns a `Map`. + - `backfillMissingFlags(logger)` — called at startup; computes rows only for flags that have none yet (no-op once all flags are populated). + - `recomputeAllFlags(logger)` — full refresh of every flag; not called automatically, available for manual recovery. +- **Assignment read path** — `FeatureFlagService.featureFlagLevelInclusionExclusion()` calls `getPrecomputedSets()` and does in-memory `Set.has()` checks. No recursive segment queries at assignment time. +- **Cache** — keyed by `CACHE_PREFIX.PRECOMPUTED_SEGMENT_KEY_PREFIX + flagId`. Invalidated by `recomputeForFlag`. + +### What triggers a recompute + +| Event | Trigger | +|---|---| +| Segment list added to a flag | `FeatureFlagService.addList` → `recomputeForFlag` | +| Segment list removed from a flag | `FeatureFlagService.deleteList` → `recomputeForFlag` | +| Segment list members updated on a flag | `FeatureFlagService.updateList` → `recomputeForFlag` | +| Private list added to a shared segment | `SegmentService.addList` → `scheduleRecomputeForSegment` | +| Private list removed from a shared segment | `SegmentService.deleteList` → `scheduleRecomputeForSegment` | +| Segment members/structure updated | `SegmentService.addSegmentDataWithPipeline` → `scheduleRecomputeForSegment` | +| Segment deleted entirely | `SegmentService.deleteSegment` — collects affected flag IDs **before** deletion, fires `recomputeForFlag` for each **after** deletion (fire-and-forget) | +| Server startup | `app.ts` → `backfillMissingFlags` — backfills any flag with no row | + +All recomputes triggered from write paths are **fire-and-forget** — callers never wait on them. + +### Key invariant + +The `precomputed_segment` row must always be recomputed **after** the structural change completes, so the flat arrays reflect the new state. For deletions specifically, affected flag IDs must be collected **before** the delete because the join table records are gone afterward. diff --git a/packages/backend/src/api/models/PrecomputedSegment.ts b/packages/backend/src/api/models/PrecomputedSegment.ts new file mode 100644 index 0000000000..e14be00605 --- /dev/null +++ b/packages/backend/src/api/models/PrecomputedSegment.ts @@ -0,0 +1,19 @@ +import { Column, Entity, JoinColumn, ManyToOne, PrimaryColumn } from 'typeorm'; +import { BaseModel } from './base/BaseModel'; +import { FeatureFlag } from './FeatureFlag'; + +@Entity() +export class PrecomputedSegment extends BaseModel { + @PrimaryColumn('uuid') + public featureFlagId: string; + + @ManyToOne(() => FeatureFlag, { onDelete: 'CASCADE' }) + @JoinColumn({ name: 'featureFlagId' }) + public featureFlag: FeatureFlag; + + @Column('text', { array: true, default: '{}' }) + public inclusionIds: string[]; + + @Column('text', { array: true, default: '{}' }) + public exclusionIds: string[]; +} diff --git a/packages/backend/src/api/repositories/FeatureFlagRepository.ts b/packages/backend/src/api/repositories/FeatureFlagRepository.ts index 0f6da24de1..c602dc8ea7 100644 --- a/packages/backend/src/api/repositories/FeatureFlagRepository.ts +++ b/packages/backend/src/api/repositories/FeatureFlagRepository.ts @@ -113,6 +113,21 @@ export class FeatureFlagRepository extends Repository { return result; } + // Minimal projection for getKeys — only id, key, filterMode needed; no segment joins + public async getFlagsForKeys(context: string): Promise[]> { + const result = await this.createQueryBuilder('feature_flag') + .select(['feature_flag.id', 'feature_flag.key', 'feature_flag.filterMode']) + .where('feature_flag.context @> :searchContext', { searchContext: [context] }) + .andWhere('feature_flag.status = :status', { status: FEATURE_FLAG_STATUS.ENABLED }) + .getMany() + .catch((errorMsg: any) => { + const errorMsgString = repositoryError('FeatureFlagRepository', 'getFlagsForKeys', { context }, errorMsg); + throw errorMsgString; + }); + + return result; + } + public async validateUniqueKey(flagDTO: FeatureFlagValidation) { const queryBuilder = this.createQueryBuilder('feature_flag') .where('feature_flag.key = :key', { key: flagDTO.key }) diff --git a/packages/backend/src/api/repositories/PrecomputedSegmentRepository.ts b/packages/backend/src/api/repositories/PrecomputedSegmentRepository.ts new file mode 100644 index 0000000000..9ac3d3cd88 --- /dev/null +++ b/packages/backend/src/api/repositories/PrecomputedSegmentRepository.ts @@ -0,0 +1,21 @@ +import { Repository } from 'typeorm'; +import { EntityRepository } from '../../typeorm-typedi-extensions'; +import { PrecomputedSegment } from '../models/PrecomputedSegment'; + +@EntityRepository(PrecomputedSegment) +export class PrecomputedSegmentRepository extends Repository { + public async upsertByFlagId(flagId: string, inclusionIds: string[], exclusionIds: string[]): Promise { + await this.createQueryBuilder() + .insert() + .into(PrecomputedSegment) + .values({ featureFlagId: flagId, inclusionIds, exclusionIds }) + .orUpdate(['inclusionIds', 'exclusionIds', 'updatedAt'], ['featureFlagId']) + .execute(); + } + + public async findByFlagIds(flagIds: string[]): Promise<(PrecomputedSegment | null)[]> { + if (!flagIds.length) return []; + const rows = await this.createQueryBuilder('ps').where('ps.featureFlagId IN (:...ids)', { ids: flagIds }).getMany(); + return flagIds.map((id) => rows.find((r) => r.featureFlagId === id) ?? null); + } +} diff --git a/packages/backend/src/api/repositories/SegmentRepository.ts b/packages/backend/src/api/repositories/SegmentRepository.ts index 2941bf50c7..2759e17efa 100644 --- a/packages/backend/src/api/repositories/SegmentRepository.ts +++ b/packages/backend/src/api/repositories/SegmentRepository.ts @@ -139,6 +139,14 @@ export class SegmentRepository extends Repository { return result.raw; } + public async findParentSegmentIds(segmentId: string): Promise { + const rows = await this.manager.query( + `SELECT "parentSegmentId" FROM "segment_for_segment" WHERE "childSegmentId" = $1`, + [segmentId] + ); + return rows.map((r: { parentSegmentId: string }) => r.parentSegmentId); + } + public async deleteSegments(ids: string[], logger: UpgradeLogger, entityManager?: EntityManager): Promise { const queryRunner = entityManager ? entityManager : this; diff --git a/packages/backend/src/api/services/FeatureFlagService.ts b/packages/backend/src/api/services/FeatureFlagService.ts index 5a6063d984..1a370821ee 100644 --- a/packages/backend/src/api/services/FeatureFlagService.ts +++ b/packages/backend/src/api/services/FeatureFlagService.ts @@ -54,6 +54,7 @@ import { SegmentRepository } from '../repositories/SegmentRepository'; import { ExperimentAuditLog } from '../models/ExperimentAuditLog'; import { NotFoundException } from '@nestjs/common/exceptions'; import { CacheService } from './CacheService'; +import { PrecomputedSegmentService } from './PrecomputedSegmentService'; import { SegmentFile, SegmentInputValidator } from '../controllers/validators/SegmentInputValidator'; import dayjs from 'dayjs'; import { getDateRangeNames } from '../repositories/utils/dateQuery'; @@ -70,7 +71,8 @@ export class FeatureFlagService { @InjectDataSource() private dataSource: DataSource, public experimentAssignmentService: ExperimentAssignmentService, public segmentService: SegmentService, - public cacheService: CacheService + public cacheService: CacheService, + public precomputedSegmentService: PrecomputedSegmentService ) {} public find(logger: UpgradeLogger): Promise { @@ -99,7 +101,7 @@ export class FeatureFlagService { throw error; } - const filteredFeatureFlags = await this.getCachedFlagsFromContext(context); + const filteredFeatureFlags = await this.getCachedFlagsForKeys(context); const includedFeatureFlags = await this.featureFlagLevelInclusionExclusion(filteredFeatureFlags, experimentUserDoc); @@ -127,9 +129,17 @@ export class FeatureFlagService { return JSON.parse(JSON.stringify(flags)); } + public async getCachedFlagsForKeys(context: string): Promise[]> { + const cacheKey = CACHE_PREFIX.FEATURE_FLAG_KEY_PREFIX + 'keys-' + context; + return this.cacheService.wrap( + cacheKey, + this.featureFlagRepository.getFlagsForKeys.bind(this.featureFlagRepository, context) + ); + } + public async clearCachedFlagsForContext(context: string): Promise { - const cacheKey = CACHE_PREFIX.FEATURE_FLAG_KEY_PREFIX + context; - return this.cacheService.delCache(cacheKey); + await this.cacheService.delCache(CACHE_PREFIX.FEATURE_FLAG_KEY_PREFIX + context); + await this.cacheService.delCache(CACHE_PREFIX.FEATURE_FLAG_KEY_PREFIX + 'keys-' + context); } public async findOne(id: string, logger?: UpgradeLogger): Promise { @@ -500,9 +510,23 @@ export class FeatureFlagService { currentUser: UserDTO, logger: UpgradeLogger ): Promise { + // Capture the flag id before deletion for recompute + const repo = + filterType === LIST_FILTER_MODE.INCLUSION + ? this.featureFlagSegmentInclusionRepository + : this.featureFlagSegmentExclusionRepository; + const record = await repo.findOne({ where: { segment: { id: segmentId } }, relations: ['featureFlag'] }); + const flagId = record?.featureFlag?.id; + await this.createDeleteListAuditLogs([segmentId], filterType, currentUser); await this.cacheService.resetPrefixCache(CACHE_PREFIX.FEATURE_FLAG_KEY_PREFIX); - return this.segmentService.deleteSegment(segmentId, logger); + const deleted = await this.segmentService.deleteSegment(segmentId, logger); + + if (flagId) { + await this.precomputedSegmentService.recomputeForFlag(flagId, logger); + } + + return deleted; } async createDeleteListAuditLogs( @@ -648,15 +672,20 @@ export class FeatureFlagService { return featureFlagSegmentInclusionOrExclusionArray; }; + let result: (FeatureFlagSegmentInclusion | FeatureFlagSegmentExclusion)[]; if (transactionalEntityManager) { - // Use the provided entity manager - return await executeTransaction(transactionalEntityManager); + result = await executeTransaction(transactionalEntityManager); } else { - // Create a new transaction if no entity manager is provided - return await this.dataSource.transaction(async (manager) => { + result = await this.dataSource.transaction(async (manager) => { return await executeTransaction(manager); }); } + + // Recompute precomputed sets for each affected flag after transaction commits + const affectedFlagIds = [...new Set(listsInput.map((l) => l.id))]; + await Promise.all(affectedFlagIds.map((flagId) => this.precomputedSegmentService.recomputeForFlag(flagId, logger))); + + return result; } public async getExposureStatsByDate( @@ -697,7 +726,7 @@ export class FeatureFlagService { ): Promise { logger.info({ message: `Update ${filterType} list for feature flag` }); await this.cacheService.resetPrefixCache(CACHE_PREFIX.FEATURE_FLAG_KEY_PREFIX); - return await this.dataSource.transaction(async (transactionalEntityManager) => { + const result = await this.dataSource.transaction(async (transactionalEntityManager) => { // Find the existing record let existingRecord: FeatureFlagSegmentInclusion | FeatureFlagSegmentExclusion; const featureFlag = await this.findOne(listInput.id); @@ -803,6 +832,10 @@ export class FeatureFlagService { return existingRecord; }); + + await this.precomputedSegmentService.recomputeForFlag(listInput.id, logger); + + return result; } private paginatedSearchString(params: IFeatureFlagSearchParams): string { @@ -859,47 +892,42 @@ export class FeatureFlagService { } private async featureFlagLevelInclusionExclusion( - featureFlags: FeatureFlag[], + featureFlags: Pick[], experimentUser: ExperimentUser - ): Promise { - const segmentObjMap = {}; - const getEnabledSegmentIds = (list: FeatureFlagSegmentExclusion[] | FeatureFlagSegmentInclusion[]) => { - return list.filter((item) => item.enabled).map((item) => item.segment.id); - }; + ): Promise[]> { + const flagIds = featureFlags.map((f) => f.id); + const precomputedMap = await this.precomputedSegmentService.getPrecomputedSets(flagIds); - featureFlags.forEach((flag) => { - const excludeIds = getEnabledSegmentIds(flag.featureFlagSegmentExclusion); - let includeIds = []; + // Flatten all group IDs from the user's group map (type is ignored per design decision) + const userGroupIds: string[] = experimentUser.group ? Object.values(experimentUser.group).flat() : []; - // this should be fixed upstream also so featureFlagSegmentInclusion is always an empty array already, - // but this will at least catch it here also if something was missed so that we aren't caching or running logic on irrelevant segments - if (flag.filterMode !== FILTER_MODE.INCLUDE_ALL) { - includeIds = getEnabledSegmentIds(flag.featureFlagSegmentInclusion); + return featureFlags.filter((flag) => { + const computed = precomputedMap.get(flag.id); + + if (!computed) { + // No precomputed row yet — apply filter_mode default conservatively + return flag.filterMode === FILTER_MODE.INCLUDE_ALL; } - segmentObjMap[flag.id] = { - segmentIdsQueue: [...includeIds, ...excludeIds], - currentIncludedSegmentIds: includeIds, - currentExcludedSegmentIds: excludeIds, - allIncludedSegmentIds: includeIds, - allExcludedSegmentIds: excludeIds, - }; - }); + const exclusionSet = new Set(computed.exclusionIds); + const inclusionSet = new Set(computed.inclusionIds); - const featureFlagIdsWithFilter: { id: string; filterMode: FILTER_MODE }[] = featureFlags.map( - ({ id, filterMode }) => ({ id, filterMode }) - ); - const [includeData, excludeData] = await this.experimentAssignmentService.resolveSegmentsForEntities(segmentObjMap); + // Individual exclusion always wins + if (exclusionSet.has(experimentUser.id)) return false; - const [includedFeatureFlagIds] = await this.experimentAssignmentService.inclusionExclusionLogic( - includeData, - excludeData, - experimentUser, - featureFlagIdsWithFilter - ); + // Individual inclusion bypasses group checks + if (inclusionSet.has(experimentUser.id)) return true; + + const inGroupExclusion = userGroupIds.some((gid) => exclusionSet.has(gid)); + const inGroupInclusion = userGroupIds.some((gid) => inclusionSet.has(gid)); - const includedFeatureFlags = featureFlags.filter(({ id }) => includedFeatureFlagIds.includes(id)); - return includedFeatureFlags; + if (flag.filterMode === FILTER_MODE.INCLUDE_ALL) { + return !inGroupExclusion; + } else { + // EXCLUDE_ALL: include only if in inclusion group and not in exclusion group + return inGroupInclusion && !inGroupExclusion; + } + }); } public async importFeatureFlags( diff --git a/packages/backend/src/api/services/PrecomputedSegmentService.ts b/packages/backend/src/api/services/PrecomputedSegmentService.ts new file mode 100644 index 0000000000..e33adf9572 --- /dev/null +++ b/packages/backend/src/api/services/PrecomputedSegmentService.ts @@ -0,0 +1,182 @@ +import { Service } from 'typedi'; +import { InjectRepository } from '../../typeorm-typedi-extensions'; +import { PrecomputedSegmentRepository } from '../repositories/PrecomputedSegmentRepository'; +import { FeatureFlagSegmentInclusionRepository } from '../repositories/FeatureFlagSegmentInclusionRepository'; +import { FeatureFlagSegmentExclusionRepository } from '../repositories/FeatureFlagSegmentExclusionRepository'; +import { FeatureFlagRepository } from '../repositories/FeatureFlagRepository'; +import { SegmentRepository } from '../repositories/SegmentRepository'; +import { PrecomputedSegment } from '../models/PrecomputedSegment'; +import { CacheService } from './CacheService'; +import { CACHE_PREFIX } from 'upgrade_types'; +import { UpgradeLogger } from '../../lib/logger/UpgradeLogger'; + +@Service() +export class PrecomputedSegmentService { + constructor( + @InjectRepository() private precomputedSegmentRepository: PrecomputedSegmentRepository, + @InjectRepository() private featureFlagSegmentInclusionRepository: FeatureFlagSegmentInclusionRepository, + @InjectRepository() private featureFlagSegmentExclusionRepository: FeatureFlagSegmentExclusionRepository, + @InjectRepository() private featureFlagRepository: FeatureFlagRepository, + @InjectRepository() private segmentRepository: SegmentRepository, + private cacheService: CacheService + ) {} + + public async recomputeForFlag(flagId: string, logger: UpgradeLogger): Promise { + const [inclusionRecords, exclusionRecords] = await Promise.all([ + this.featureFlagSegmentInclusionRepository.find({ + where: { featureFlag: { id: flagId }, enabled: true }, + relations: ['segment'], + }), + this.featureFlagSegmentExclusionRepository.find({ + where: { featureFlag: { id: flagId }, enabled: true }, + relations: ['segment'], + }), + ]); + + const inclusionSegmentIds = inclusionRecords.map((r) => r.segment.id); + const exclusionSegmentIds = exclusionRecords.map((r) => r.segment.id); + + const [inclusionIds, exclusionIds] = await Promise.all([ + this.flattenSegmentMembers(inclusionSegmentIds, new Set()), + this.flattenSegmentMembers(exclusionSegmentIds, new Set()), + ]); + + await this.precomputedSegmentRepository.upsertByFlagId( + flagId, + [...new Set(inclusionIds)], + [...new Set(exclusionIds)] + ); + + await this.cacheService.delCache(CACHE_PREFIX.PRECOMPUTED_SEGMENT_KEY_PREFIX + flagId); + logger.info({ message: `Recomputed precomputed_segment for flag ${flagId}` }); + } + + // Triggered on segment member or structure changes — finds all affected flags and recomputes (fire-and-forget) + public scheduleRecomputeForSegment(segmentId: string, logger: UpgradeLogger): void { + this.collectAffectedFlagIds(segmentId, new Set()) + .then((flagIds) => Promise.all([...flagIds].map((flagId) => this.recomputeForFlag(flagId, logger)))) + .catch((err) => logger.error({ message: `Error in scheduleRecomputeForSegment: ${err}` })); + } + + public async getPrecomputedSets(flagIds: string[]): Promise> { + if (!flagIds.length) return new Map(); + + const results = await this.cacheService.wrapFunction(CACHE_PREFIX.PRECOMPUTED_SEGMENT_KEY_PREFIX, flagIds, () => + this.precomputedSegmentRepository.findByFlagIds(flagIds) + ); + + const map = new Map(); + flagIds.forEach((id, i) => { + if (results[i]) map.set(id, results[i] as PrecomputedSegment); + }); + return map; + } + + // One-time backfill for all existing flags — call at startup or after migration + public async recomputeAllFlags(logger: UpgradeLogger): Promise { + const flags = await this.featureFlagRepository.find({ select: ['id'] }); + for (const flag of flags) { + try { + await this.recomputeForFlag(flag.id, logger); + } catch (err) { + logger.error({ message: `Failed to recompute precomputed_segment for flag ${flag.id}: ${err}` }); + } + } + logger.info({ message: `Backfill complete: recomputed ${flags.length} flags` }); + } + + // Backfill only flags that have no precomputed_segment row yet — safe to run every startup + public async backfillMissingFlags(logger: UpgradeLogger): Promise { + const [allFlags, existingRows] = await Promise.all([ + this.featureFlagRepository.find({ select: ['id'] }), + this.precomputedSegmentRepository.find({ select: ['featureFlagId'] }), + ]); + + const existingFlagIds = new Set(existingRows.map((r) => r.featureFlagId)); + const missingFlags = allFlags.filter((f) => !existingFlagIds.has(f.id)); + + if (!missingFlags.length) { + logger.info({ message: 'precomputed_segment backfill: all flags already have rows, nothing to do' }); + return; + } + + for (const flag of missingFlags) { + try { + await this.recomputeForFlag(flag.id, logger); + } catch (err) { + logger.error({ message: `Failed to backfill precomputed_segment for flag ${flag.id}: ${err}` }); + } + } + logger.info({ + message: `precomputed_segment backfill complete: computed ${missingFlags.length} of ${allFlags.length} flags`, + }); + } + + private async flattenSegmentMembers(segmentIds: string[], seen: Set): Promise { + const unresolved = segmentIds.filter((id) => !seen.has(id)); + if (!unresolved.length) return []; + + unresolved.forEach((id) => seen.add(id)); + + const segments = await this.segmentRepository + .createQueryBuilder('segment') + .leftJoinAndSelect('segment.individualForSegment', 'individual') + .leftJoinAndSelect('segment.groupForSegment', 'group') + .leftJoinAndSelect('segment.subSegments', 'subSegment') + .where('segment.id IN (:...ids)', { ids: unresolved }) + .getMany(); + + const ids: string[] = []; + const subSegmentIds: string[] = []; + + for (const segment of segments) { + segment.individualForSegment.forEach((ind) => ids.push(ind.userId)); + segment.groupForSegment.forEach((grp) => ids.push(grp.groupId)); + segment.subSegments.forEach((sub) => { + if (!seen.has(sub.id)) subSegmentIds.push(sub.id); + }); + } + + if (subSegmentIds.length) { + const subIds = await this.flattenSegmentMembers(subSegmentIds, seen); + ids.push(...subIds); + } + + return ids; + } + + public async getAffectedFlagIds(segmentId: string): Promise { + return [...(await this.collectAffectedFlagIds(segmentId, new Set()))]; + } + + private async collectAffectedFlagIds(segmentId: string, visited: Set): Promise> { + if (visited.has(segmentId)) return new Set(); + visited.add(segmentId); + + const [inclusionRecords, exclusionRecords] = await Promise.all([ + this.featureFlagSegmentInclusionRepository.find({ + where: { segment: { id: segmentId } }, + relations: ['featureFlag'], + }), + this.featureFlagSegmentExclusionRepository.find({ + where: { segment: { id: segmentId } }, + relations: ['featureFlag'], + }), + ]); + + const flagIds = new Set([ + ...inclusionRecords.map((r) => r.featureFlag.id), + ...exclusionRecords.map((r) => r.featureFlag.id), + ]); + + const parentIds = await this.segmentRepository.findParentSegmentIds(segmentId); + await Promise.all( + parentIds.map(async (parentId) => { + const parentFlagIds = await this.collectAffectedFlagIds(parentId, visited); + parentFlagIds.forEach((id) => flagIds.add(id)); + }) + ); + + return flagIds; + } +} diff --git a/packages/backend/src/api/services/SegmentService.ts b/packages/backend/src/api/services/SegmentService.ts index a8aaf9f4bf..87a54b359d 100644 --- a/packages/backend/src/api/services/SegmentService.ts +++ b/packages/backend/src/api/services/SegmentService.ts @@ -37,6 +37,7 @@ import { FeatureFlagSegmentExclusionRepository } from '../repositories/FeatureFl import { FeatureFlagSegmentInclusionRepository } from '../repositories/FeatureFlagSegmentInclusionRepository'; import { getSegmentData, getSegmentsData } from '../controllers/SegmentController'; import { CacheService } from './CacheService'; +import { PrecomputedSegmentService } from './PrecomputedSegmentService'; import { isUUID, validate } from 'class-validator'; import { plainToClass } from 'class-transformer'; import path from 'path'; @@ -83,7 +84,8 @@ export class SegmentService { private featureFlagSegmentExclusionRepository: FeatureFlagSegmentExclusionRepository, @InjectRepository() private featureFlagSegmentInclusionRepository: FeatureFlagSegmentInclusionRepository, - private cacheService: CacheService + private cacheService: CacheService, + private precomputedSegmentService: PrecomputedSegmentService ) {} public async getAllSegments(logger: UpgradeLogger): Promise { @@ -434,6 +436,9 @@ export class SegmentService { await transactionalEntityManager.getRepository(Segment).save(parentSegment); return createdSegment; }); + + this.precomputedSegmentService.scheduleRecomputeForSegment(parentSegmentId, logger); + return createdSegment; } @@ -459,6 +464,9 @@ export class SegmentService { await transactionalEntityManager.getRepository(Segment).save(parentSegment); return deletedSegmentResponse; }); + + this.precomputedSegmentService.scheduleRecomputeForSegment(parentSegmentId, logger); + return deletedSegmentResponse[0]; } @@ -473,10 +481,17 @@ export class SegmentService { public async deleteSegment(id: string, logger: UpgradeLogger): Promise { logger.info({ message: `Delete segment by id. segmentId: ${id}` }); + // Collect affected flags before deletion — join table records are gone after + const affectedFlagIds = await this.precomputedSegmentService.getAffectedFlagIds(id); + const manager = this.dataSource; const deletedSegment = manager.transaction(async (transactionalEntityManager) => { return this.deleteSegmentAndPrivateSubsegments(id, logger, transactionalEntityManager); }); + + // Recompute after deletion so stale member IDs are removed (fire-and-forget) + affectedFlagIds.forEach((flagId) => this.precomputedSegmentService.recomputeForFlag(flagId, logger)); + return deletedSegment; } @@ -1015,6 +1030,9 @@ export class SegmentService { // reset cache await this.cacheService.resetPrefixCache(CACHE_PREFIX.SEGMENT_KEY_PREFIX); + // Recompute precomputed sets for all flags that reference this segment (fire-and-forget) + this.precomputedSegmentService.scheduleRecomputeForSegment(segmentDoc.id, logger); + return transactionalEntityManager .getRepository(Segment) .findOne({ where: { id: segmentDoc.id }, relations: ['individualForSegment', 'groupForSegment', 'subSegments'] }); diff --git a/packages/backend/src/app.ts b/packages/backend/src/app.ts index 4e579ef0b5..133ef132dd 100644 --- a/packages/backend/src/app.ts +++ b/packages/backend/src/app.ts @@ -19,6 +19,7 @@ import { enableMetricFiltering } from './init/seed/EnableMetricFiltering'; import { InitMetrics } from './init/seed/initMetrics'; import { banner } from './lib/banner'; import { createGlobalExcludeSegment } from './init/seed/globalExcludeSegment'; +import { backfillPrecomputedSegments } from './init/seed/backfillPrecomputedSegments'; /* * EXPRESS TYPESCRIPT BOILERPLATE @@ -47,4 +48,7 @@ bootstrapMicroframework({ .then(() => { // Create global exclude segment return createGlobalExcludeSegment(logger); + }) + .then(() => { + return backfillPrecomputedSegments(logger); }); diff --git a/packages/backend/src/database/migrations/1779500000000-precomputedSegment.ts b/packages/backend/src/database/migrations/1779500000000-precomputedSegment.ts new file mode 100644 index 0000000000..8947c78d4a --- /dev/null +++ b/packages/backend/src/database/migrations/1779500000000-precomputedSegment.ts @@ -0,0 +1,25 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; + +export class PrecomputedSegment1779500000000 implements MigrationInterface { + name = 'PrecomputedSegment1779500000000'; + + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.query(` + CREATE TABLE "precomputed_segment" ( + "featureFlagId" uuid NOT NULL, + "inclusionIds" text[] NOT NULL DEFAULT '{}', + "exclusionIds" text[] NOT NULL DEFAULT '{}', + "createdAt" TIMESTAMP NOT NULL DEFAULT now(), + "updatedAt" TIMESTAMP NOT NULL DEFAULT now(), + "versionNumber" integer NOT NULL DEFAULT 1, + CONSTRAINT "PK_precomputed_segment" PRIMARY KEY ("featureFlagId"), + CONSTRAINT "FK_precomputed_segment_feature_flag" + FOREIGN KEY ("featureFlagId") REFERENCES "feature_flag"("id") ON DELETE CASCADE + ) + `); + } + + public async down(queryRunner: QueryRunner): Promise { + await queryRunner.query(`DROP TABLE "precomputed_segment"`); + } +} diff --git a/packages/backend/src/init/seed/backfillPrecomputedSegments.ts b/packages/backend/src/init/seed/backfillPrecomputedSegments.ts new file mode 100644 index 0000000000..1ab6686ac4 --- /dev/null +++ b/packages/backend/src/init/seed/backfillPrecomputedSegments.ts @@ -0,0 +1,8 @@ +import { PrecomputedSegmentService } from '../../api/services/PrecomputedSegmentService'; +import { UpgradeLogger } from '../../lib/logger/UpgradeLogger'; +import Container from 'typedi'; + +export async function backfillPrecomputedSegments(logger: UpgradeLogger): Promise { + const precomputedSegmentService = Container.get(PrecomputedSegmentService); + await precomputedSegmentService.backfillMissingFlags(logger); +} diff --git a/packages/types/src/Experiment/enums.ts b/packages/types/src/Experiment/enums.ts index f6f8a55540..a6180c43ec 100644 --- a/packages/types/src/Experiment/enums.ts +++ b/packages/types/src/Experiment/enums.ts @@ -338,6 +338,7 @@ export enum CACHE_PREFIX { SEGMENT_KEY_PREFIX = 'segments-', MARK_KEY_PREFIX = 'markExperiments-', FEATURE_FLAG_KEY_PREFIX = 'featureFlags-', + PRECOMPUTED_SEGMENT_KEY_PREFIX = 'precomputedSegments-', } export enum STATUS_INDICATOR_CHIP_TYPE { From 3925f2fe2b71f43f0a93c0275ce9cce85fd862f1 Mon Sep 17 00:00:00 2001 From: doswalt Date: Thu, 25 Jun 2026 16:50:47 -0400 Subject: [PATCH 02/16] handle a bug with toggling enable/disable on a large list in feature-flag include lists --- .../repositories/GroupForSegmentRepository.ts | 50 ++++++++++++------- .../IndividualForSegmentRepository.ts | 50 ++++++++++++------- .../src/api/services/FeatureFlagService.ts | 7 ++- .../src/api/services/SegmentService.ts | 42 +++++++++------- 4 files changed, 90 insertions(+), 59 deletions(-) diff --git a/packages/backend/src/api/repositories/GroupForSegmentRepository.ts b/packages/backend/src/api/repositories/GroupForSegmentRepository.ts index d9960607df..34e9f9d64b 100644 --- a/packages/backend/src/api/repositories/GroupForSegmentRepository.ts +++ b/packages/backend/src/api/repositories/GroupForSegmentRepository.ts @@ -28,26 +28,38 @@ export class GroupForSegmentRepository extends Repository { entityManager: EntityManager, logger: UpgradeLogger ): Promise { - const result = await entityManager - .createQueryBuilder() - .insert() - .into(GroupForSegment) - .values(data) - .orIgnore() - .returning('*') - .execute() - .catch((errorMsg: any) => { - const errorMsgString = repositoryError( - 'groupForSegmentRepository', - 'insertGroupForSegment', - { data }, - errorMsg - ); - logger.error(errorMsg); - throw errorMsgString; - }); + if (!data.length) return []; - return result.raw; + // PostgreSQL's wire protocol supports at most 65535 bind parameters per statement. + // GroupForSegment has 3 bound columns (segmentId, groupId, type), so cap at 5000 rows + // per chunk (5000 × 3 = 15000, well under the limit). + const CHUNK_SIZE = 5000; + const results: GroupForSegment[] = []; + + for (let i = 0; i < data.length; i += CHUNK_SIZE) { + const chunk = data.slice(i, i + CHUNK_SIZE); + const result = await entityManager + .createQueryBuilder() + .insert() + .into(GroupForSegment) + .values(chunk) + .orIgnore() + .returning('*') + .execute() + .catch((errorMsg: any) => { + const errorMsgString = repositoryError( + 'groupForSegmentRepository', + 'insertGroupForSegment', + { data: chunk }, + errorMsg + ); + logger.error(errorMsg); + throw errorMsgString; + }); + results.push(...result.raw); + } + + return results; } public async deleteGroupForSegment( diff --git a/packages/backend/src/api/repositories/IndividualForSegmentRepository.ts b/packages/backend/src/api/repositories/IndividualForSegmentRepository.ts index 85030de66c..507ab30323 100644 --- a/packages/backend/src/api/repositories/IndividualForSegmentRepository.ts +++ b/packages/backend/src/api/repositories/IndividualForSegmentRepository.ts @@ -29,26 +29,38 @@ export class IndividualForSegmentRepository extends Repository { - const result = await entityManager - .createQueryBuilder() - .insert() - .into(IndividualForSegment) - .values(data) - .orIgnore() - .returning('*') - .execute() - .catch((errorMsg: any) => { - const errorMsgString = repositoryError( - 'individualForSegmentRepository', - 'insertIndividualForSegment', - { data }, - errorMsg - ); - logger.error(errorMsg); - throw errorMsgString; - }); + if (!data.length) return []; - return result.raw; + // PostgreSQL's wire protocol supports at most 65535 bind parameters per statement. + // IndividualForSegment has 2 bound columns (segmentId, userId), so cap at 5000 rows + // per chunk (5000 × 2 = 10000, well under the limit). + const CHUNK_SIZE = 5000; + const results: IndividualForSegment[] = []; + + for (let i = 0; i < data.length; i += CHUNK_SIZE) { + const chunk = data.slice(i, i + CHUNK_SIZE); + const result = await entityManager + .createQueryBuilder() + .insert() + .into(IndividualForSegment) + .values(chunk) + .orIgnore() + .returning('*') + .execute() + .catch((errorMsg: any) => { + const errorMsgString = repositoryError( + 'individualForSegmentRepository', + 'insertIndividualForSegment', + { data: chunk }, + errorMsg + ); + logger.error(errorMsg); + throw errorMsgString; + }); + results.push(...result.raw); + } + + return results; } public async deleteIndividualForSegment( diff --git a/packages/backend/src/api/services/FeatureFlagService.ts b/packages/backend/src/api/services/FeatureFlagService.ts index 1a370821ee..b9df7f36cf 100644 --- a/packages/backend/src/api/services/FeatureFlagService.ts +++ b/packages/backend/src/api/services/FeatureFlagService.ts @@ -759,12 +759,15 @@ export class FeatureFlagService { const oldSegmentDocClone = JSON.parse(JSON.stringify(oldSegmentDoc)); let newSegmentDocClone; - // Update the segment + // Update the segment. Pass skipScheduleRecompute=true because updateList calls + // recomputeForFlag explicitly after the transaction — firing scheduleRecomputeForSegment + // from inside the transaction risks a stale-read race on the enabled flag. try { const updatedSegment = await this.segmentService.upsertSegmentInPipeline( listInput.segment, logger, - transactionalEntityManager + transactionalEntityManager, + true ); existingRecord.segment = updatedSegment; diff --git a/packages/backend/src/api/services/SegmentService.ts b/packages/backend/src/api/services/SegmentService.ts index 87a54b359d..cf8d18d147 100644 --- a/packages/backend/src/api/services/SegmentService.ts +++ b/packages/backend/src/api/services/SegmentService.ts @@ -41,8 +41,6 @@ import { PrecomputedSegmentService } from './PrecomputedSegmentService'; import { isUUID, validate } from 'class-validator'; import { plainToClass } from 'class-transformer'; import path from 'path'; -import { IndividualForSegment } from '../models/IndividualForSegment'; -import { GroupForSegment } from '../models/GroupForSegment'; import { ISegmentSearchParams, ISegmentSortParams } from '../controllers/validators/SegmentPaginatedParamsValidator'; import { ExperimentSegmentExclusion } from 'src/api/models/ExperimentSegmentExclusion'; import { ExperimentSegmentInclusion } from 'src/api/models/ExperimentSegmentInclusion'; @@ -473,10 +471,11 @@ export class SegmentService { public upsertSegmentInPipeline( segment: SegmentInputValidator, logger: UpgradeLogger, - transactionalEntityManager: EntityManager + transactionalEntityManager: EntityManager, + skipScheduleRecompute = false ): Promise { logger.info({ message: `Upsert segment => ${JSON.stringify(segment, undefined, 2)}` }); - return this.addSegmentDataWithPipeline(segment, logger, transactionalEntityManager); + return this.addSegmentDataWithPipeline(segment, logger, transactionalEntityManager, skipScheduleRecompute); } public async deleteSegment(id: string, logger: UpgradeLogger): Promise { @@ -890,12 +889,11 @@ export class SegmentService { async addSegmentDataWithPipeline( segment: SegmentInputValidator, logger: UpgradeLogger, - transactionalEntityManager: EntityManager + transactionalEntityManager: EntityManager, + skipScheduleRecompute = false ): Promise { let segmentDoc: Segment; - let usersToDelete = [], - groupsToDelete = []; if (segment.id) { try { // get segment by ids @@ -904,20 +902,21 @@ export class SegmentService { relations: ['individualForSegment', 'groupForSegment', 'subSegments'], }); - // delete individual for segment + // delete all members for this segment by segment id (single-param query, no per-row overhead) if (segmentDoc && segmentDoc.individualForSegment && segmentDoc.individualForSegment.length > 0) { - usersToDelete = segmentDoc.individualForSegment.map((individual) => { - return { userId: individual.userId, segment: segment }; - }); - await transactionalEntityManager.getRepository(IndividualForSegment).delete(usersToDelete as any); + await this.individualForSegmentRepository.deleteIndividualForSegmentById( + segment.id, + transactionalEntityManager, + logger + ); } - // delete group for segment if (segmentDoc && segmentDoc.groupForSegment && segmentDoc.groupForSegment.length > 0) { - groupsToDelete = segmentDoc.groupForSegment.map((group) => { - return { groupId: group.groupId, type: group.type, segment: segment }; - }); - await transactionalEntityManager.getRepository(GroupForSegment).delete(groupsToDelete as any); + await this.groupForSegmentRepository.deleteGroupForSegmentById( + segment.id, + transactionalEntityManager, + logger + ); } } catch (err) { const error = err as ErrorWithType; @@ -1030,8 +1029,13 @@ export class SegmentService { // reset cache await this.cacheService.resetPrefixCache(CACHE_PREFIX.SEGMENT_KEY_PREFIX); - // Recompute precomputed sets for all flags that reference this segment (fire-and-forget) - this.precomputedSegmentService.scheduleRecomputeForSegment(segmentDoc.id, logger); + // Recompute precomputed sets for all flags that reference this segment (fire-and-forget). + // Skip when the caller already owns an explicit recomputeForFlag after the transaction — + // firing this from inside a transaction risks a stale-read race where the fire-and-forget + // reads the old enabled value and its upsert overwrites the correct post-commit result. + if (!skipScheduleRecompute) { + this.precomputedSegmentService.scheduleRecomputeForSegment(segmentDoc.id, logger); + } return transactionalEntityManager .getRepository(Segment) From 957a12738858bcac796eca21302cdf6f98f62f13 Mon Sep 17 00:00:00 2001 From: doswalt Date: Fri, 26 Jun 2026 15:19:48 -0400 Subject: [PATCH 03/16] fixes seeding blank row and closing gaps for stale data --- .../src/api/services/FeatureFlagService.ts | 107 ++++++++++++++++-- .../api/services/PrecomputedSegmentService.ts | 16 +++ .../src/api/services/SegmentService.ts | 3 +- 3 files changed, 117 insertions(+), 9 deletions(-) diff --git a/packages/backend/src/api/services/FeatureFlagService.ts b/packages/backend/src/api/services/FeatureFlagService.ts index b9df7f36cf..8568ac4d1e 100644 --- a/packages/backend/src/api/services/FeatureFlagService.ts +++ b/packages/backend/src/api/services/FeatureFlagService.ts @@ -103,7 +103,12 @@ export class FeatureFlagService { const filteredFeatureFlags = await this.getCachedFlagsForKeys(context); - const includedFeatureFlags = await this.featureFlagLevelInclusionExclusion(filteredFeatureFlags, experimentUserDoc); + const includedFeatureFlags = await this.featureFlagLevelInclusionExclusion( + filteredFeatureFlags, + experimentUserDoc, + context, + logger + ); // save exposures in db if (includedFeatureFlags.length > 0) { @@ -405,6 +410,12 @@ export class FeatureFlagService { flagName: featureFlagDoc.name, }; await this.experimentAuditLogRepository.saveRawJson(LOG_TYPE.FEATURE_FLAG_CREATED, createAuditLogData, user); + + // Seed an empty precomputed_segment row in the same transaction so the new flag always + // has a row (no segment lists yet => empty arrays). This keeps the assignment read path + // off the on-the-fly fallback for the common case and keeps the getKeys cache effective. + await this.precomputedSegmentService.seedEmptyRowForFlag(featureFlagDoc.id, manager); + return featureFlagDoc; }; @@ -674,16 +685,22 @@ export class FeatureFlagService { let result: (FeatureFlagSegmentInclusion | FeatureFlagSegmentExclusion)[]; if (transactionalEntityManager) { + // The caller owns the outer transaction. We must NOT recompute here: recomputeForFlag + // reads through its own repositories and cannot see this transaction's uncommitted writes, + // so it would persist an empty/stale precomputed_segment row that never self-heals. The + // caller is responsible for calling recomputeForFlag after its transaction commits. result = await executeTransaction(transactionalEntityManager); } else { result = await this.dataSource.transaction(async (manager) => { return await executeTransaction(manager); }); - } - // Recompute precomputed sets for each affected flag after transaction commits - const affectedFlagIds = [...new Set(listsInput.map((l) => l.id))]; - await Promise.all(affectedFlagIds.map((flagId) => this.precomputedSegmentService.recomputeForFlag(flagId, logger))); + // Recompute precomputed sets for each affected flag after the transaction commits + const affectedFlagIds = [...new Set(listsInput.map((l) => l.id))]; + await Promise.all( + affectedFlagIds.map((flagId) => this.precomputedSegmentService.recomputeForFlag(flagId, logger)) + ); + } return result; } @@ -896,7 +913,9 @@ export class FeatureFlagService { private async featureFlagLevelInclusionExclusion( featureFlags: Pick[], - experimentUser: ExperimentUser + experimentUser: ExperimentUser, + context: string, + logger: UpgradeLogger ): Promise[]> { const flagIds = featureFlags.map((f) => f.id); const precomputedMap = await this.precomputedSegmentService.getPrecomputedSets(flagIds); @@ -904,12 +923,21 @@ export class FeatureFlagService { // Flatten all group IDs from the user's group map (type is ignored per design decision) const userGroupIds: string[] = experimentUser.group ? Object.values(experimentUser.group).flat() : []; + // Any flag without a precomputed row falls back to on-the-fly segment resolution so a + // truly-missing row never silently produces a wrong include/exclude decision. Seeding on + // create (and recompute on every list mutation) should make this rare — log it so a + // persistent fallback is visible rather than silently masking a recompute gap. + const missingFlagIds = flagIds.filter((id) => !precomputedMap.has(id)); + const onTheFlyIncludedIds = missingFlagIds.length + ? await this.resolveFlagsOnTheFly(missingFlagIds, context, experimentUser, logger) + : new Set(); + return featureFlags.filter((flag) => { const computed = precomputedMap.get(flag.id); if (!computed) { - // No precomputed row yet — apply filter_mode default conservatively - return flag.filterMode === FILTER_MODE.INCLUDE_ALL; + // No precomputed row — fall back to the on-the-fly resolution result for this flag + return onTheFlyIncludedIds.has(flag.id); } const exclusionSet = new Set(computed.exclusionIds); @@ -933,6 +961,61 @@ export class FeatureFlagService { }); } + /** + * Fallback assignment path for flags that have no precomputed_segment row. Resolves segment + * inclusion/exclusion on-the-fly using the same recursive resolution the codebase used before + * precomputed segments (and that experiments still use), preserving full group-type matching. + * Returns the set of flag IDs the user should be included in. + */ + private async resolveFlagsOnTheFly( + missingFlagIds: string[], + context: string, + experimentUser: ExperimentUser, + logger: UpgradeLogger + ): Promise> { + logger.warn({ + message: `featureFlagLevelInclusionExclusion: ${missingFlagIds.length} flag(s) missing a precomputed_segment row; resolving on-the-fly`, + details: { context, missingFlagIds }, + }); + + // Load the full flags (with segment relations) for this context and keep only the missing ones + const missingIdSet = new Set(missingFlagIds); + const fullFlags = (await this.getCachedFlagsFromContext(context)).filter((flag) => missingIdSet.has(flag.id)); + if (!fullFlags.length) { + return new Set(); + } + + const getEnabledSegmentIds = (list: (FeatureFlagSegmentExclusion | FeatureFlagSegmentInclusion)[]) => + (list ?? []).filter((item) => item.enabled).map((item) => item.segment.id); + + const segmentObjMap: Record = {}; + fullFlags.forEach((flag) => { + const excludeIds = getEnabledSegmentIds(flag.featureFlagSegmentExclusion); + // INCLUDE_ALL flags ignore inclusion segments (matches the precomputed-path semantics) + const includeIds = + flag.filterMode !== FILTER_MODE.INCLUDE_ALL ? getEnabledSegmentIds(flag.featureFlagSegmentInclusion) : []; + + segmentObjMap[flag.id] = { + segmentIdsQueue: [...includeIds, ...excludeIds], + currentIncludedSegmentIds: includeIds, + currentExcludedSegmentIds: excludeIds, + allIncludedSegmentIds: includeIds, + allExcludedSegmentIds: excludeIds, + }; + }); + + const flagIdsWithFilter = fullFlags.map(({ id, filterMode }) => ({ id, filterMode })); + const [includeData, excludeData] = await this.experimentAssignmentService.resolveSegmentsForEntities(segmentObjMap); + const [includedFlagIds] = await this.experimentAssignmentService.inclusionExclusionLogic( + includeData, + excludeData, + experimentUser, + flagIdsWithFilter + ); + + return new Set(includedFlagIds); + } + public async importFeatureFlags( featureFlagFiles: IImportFile[], currentUser: UserDTO, @@ -1088,6 +1171,10 @@ export class FeatureFlagService { }); createdFlags.push(createdFlag); + + // The outer transaction has committed — recompute now (addList skipped it because it ran + // inside the transaction) so the imported enabled lists are reflected in precomputed_segment. + await this.precomputedSegmentService.recomputeForFlag(createdFlag.id, logger); } logger.info({ message: 'Imported feature flags', details: createdFlags }); @@ -1281,6 +1368,10 @@ export class FeatureFlagService { return await this.addList(listDocs, filterType, currentUser, logger, transactionalEntityManager); }); + // The outer transaction has committed — recompute now (addList skipped it because it ran + // inside the transaction) so the imported lists are reflected in precomputed_segment. + await this.precomputedSegmentService.recomputeForFlag(featureFlagId, logger); + logger.info({ message: 'Imported feature flags', details: createdLists }); fileStatusArray.forEach((fileStatus) => { diff --git a/packages/backend/src/api/services/PrecomputedSegmentService.ts b/packages/backend/src/api/services/PrecomputedSegmentService.ts index e33adf9572..5545f0f13b 100644 --- a/packages/backend/src/api/services/PrecomputedSegmentService.ts +++ b/packages/backend/src/api/services/PrecomputedSegmentService.ts @@ -9,6 +9,7 @@ import { PrecomputedSegment } from '../models/PrecomputedSegment'; import { CacheService } from './CacheService'; import { CACHE_PREFIX } from 'upgrade_types'; import { UpgradeLogger } from '../../lib/logger/UpgradeLogger'; +import { EntityManager } from 'typeorm'; @Service() export class PrecomputedSegmentService { @@ -51,6 +52,21 @@ export class PrecomputedSegmentService { logger.info({ message: `Recomputed precomputed_segment for flag ${flagId}` }); } + // Seed an empty precomputed_segment row for a brand-new flag, inside the flag's own + // creation transaction so the row is atomic with the flag insert. A new flag has no + // segment lists, so empty arrays are the correct initial state. `orIgnore` keeps this a + // no-op if a row somehow already exists. Seeding here guarantees getPrecomputedSets never + // returns a missing row for a freshly created flag (keeps the read-path cache effective). + public async seedEmptyRowForFlag(flagId: string, manager: EntityManager): Promise { + await manager + .createQueryBuilder() + .insert() + .into(PrecomputedSegment) + .values({ featureFlagId: flagId, inclusionIds: [], exclusionIds: [] }) + .orIgnore() + .execute(); + } + // Triggered on segment member or structure changes — finds all affected flags and recomputes (fire-and-forget) public scheduleRecomputeForSegment(segmentId: string, logger: UpgradeLogger): void { this.collectAffectedFlagIds(segmentId, new Set()) diff --git a/packages/backend/src/api/services/SegmentService.ts b/packages/backend/src/api/services/SegmentService.ts index eb159de32b..e98ad1894f 100644 --- a/packages/backend/src/api/services/SegmentService.ts +++ b/packages/backend/src/api/services/SegmentService.ts @@ -487,7 +487,8 @@ export class SegmentService { return this.deleteSegmentAndPrivateSubsegments(id, logger, transactionalEntityManager); }); - // Recompute after deletion so stale member IDs are removed (fire-and-forget) + // Recompute after the delete transaction has committed so the recompute reads the + // post-delete state and stale member IDs are removed (fire-and-forget). affectedFlagIds.forEach((flagId) => this.precomputedSegmentService.recomputeForFlag(flagId, logger)); // reset cache From 833846d8cb9ca4f81bbbb0d8a88fe21a546ba368 Mon Sep 17 00:00:00 2001 From: doswalt Date: Fri, 26 Jun 2026 17:01:30 -0400 Subject: [PATCH 04/16] test updates for latest changes --- .../unit/services/FeatureFlagService.test.ts | 98 ++++++++ .../PrecomputedSegmentService.test.ts | 209 ++++++++++++++++++ .../test/unit/services/SegmentService.test.ts | 41 ++++ 3 files changed, 348 insertions(+) create mode 100644 packages/backend/test/unit/services/PrecomputedSegmentService.test.ts diff --git a/packages/backend/test/unit/services/FeatureFlagService.test.ts b/packages/backend/test/unit/services/FeatureFlagService.test.ts index a18840cfdf..3847b37613 100644 --- a/packages/backend/test/unit/services/FeatureFlagService.test.ts +++ b/packages/backend/test/unit/services/FeatureFlagService.test.ts @@ -28,6 +28,7 @@ import { ExperimentAssignmentService } from '../../../src/api/services/Experimen import { FeatureFlagValidation } from '../../../src/api/controllers/validators/FeatureFlagValidator'; import { FeatureFlagListValidator } from '../../../src/api/controllers/validators/FeatureFlagListValidator'; import { SegmentService } from '../../../src/api/services/SegmentService'; +import { PrecomputedSegmentService } from '../../../src/api/services/PrecomputedSegmentService'; import { FeatureFlagSegmentExclusionRepository } from '../../../src/api/repositories/FeatureFlagSegmentExclusionRepository'; import { FeatureFlagSegmentInclusionRepository } from '../../../src/api/repositories/FeatureFlagSegmentInclusionRepository'; import { FeatureFlagExposureRepository } from '../../../src/api/repositories/FeatureFlagExposureRepository'; @@ -203,11 +204,26 @@ describe('Feature Flag Service Testing', () => { getSegmentByIds: jest.fn().mockResolvedValue([mockSegment]), }, }, + { + provide: PrecomputedSegmentService, + useValue: { + // Empty map by default => every flag is "missing" a precomputed row, so getKeys + // routes through the on-the-fly fallback (resolveSegmentsForEntities/inclusionExclusionLogic). + // Individual tests override getPrecomputedSets to exercise the fast in-memory path. + getPrecomputedSets: jest.fn().mockResolvedValue(new Map()), + recomputeForFlag: jest.fn().mockResolvedValue(undefined), + seedEmptyRowForFlag: jest.fn().mockResolvedValue(undefined), + scheduleRecomputeForSegment: jest.fn(), + getAffectedFlagIds: jest.fn().mockResolvedValue([]), + }, + }, { provide: getRepositoryToken(FeatureFlagRepository), useValue: { find: jest.fn().mockResolvedValue(mockFlagArr), findBy: jest.fn().mockResolvedValue(mockFlagArr), + getFlagsForKeys: jest.fn().mockResolvedValue(mockFlagArr), + getFlagsFromContext: jest.fn().mockResolvedValue(mockFlagArr), findOne: jest.fn().mockResolvedValue(mockFlag1), findWithNames: jest.fn().mockResolvedValue(mockFlagArr), findOneById: jest.fn().mockResolvedValue(mockFlag1), @@ -682,4 +698,86 @@ describe('Feature Flag Service Testing', () => { expect(segmentObjMap[flagWithDisabledInclusion.id].currentIncludedSegmentIds).toEqual([]); }); }); + + describe('getKeys - precomputed segment fast path', () => { + const fastFlag = { id: 'fast-flag-id', key: 'fast-key', filterMode: FILTER_MODE.INCLUDE_ALL }; + + it('uses the precomputed set and skips on-the-fly resolution when a row exists', async () => { + const userDoc = { id: 'user123', group: {}, workingGroup: {} } as any; + const experimentAssignmentService = module.get(ExperimentAssignmentService); + const precomputed = module.get(PrecomputedSegmentService); + + service.cacheService.wrap = jest.fn().mockResolvedValue([fastFlag]); + (precomputed.getPrecomputedSets as jest.Mock).mockResolvedValue( + new Map([[fastFlag.id, { inclusionIds: [], exclusionIds: ['user123'] }]]) + ); + + const result = await service.getKeys(userDoc, 'context1', logger); + + // user123 is individually excluded -> flag filtered out + expect(result).toEqual([]); + // fast path must not fall back to recursive resolution + expect(experimentAssignmentService.resolveSegmentsForEntities).not.toHaveBeenCalled(); + }); + + it('individual inclusion beats group exclusion on the fast path', async () => { + const userDoc = { id: 'user123', group: { classId: ['bad-class'] }, workingGroup: {} } as any; + const precomputed = module.get(PrecomputedSegmentService); + + service.cacheService.wrap = jest.fn().mockResolvedValue([fastFlag]); + (precomputed.getPrecomputedSets as jest.Mock).mockResolvedValue( + new Map([[fastFlag.id, { inclusionIds: ['user123'], exclusionIds: ['bad-class'] }]]) + ); + + const result = await service.getKeys(userDoc, 'context1', logger); + + expect(result).toEqual([fastFlag.key]); + }); + + it('falls back to on-the-fly resolution when the precomputed row is missing', async () => { + const userDoc = { id: 'user123', group: {}, workingGroup: {} } as any; + const experimentAssignmentService = module.get(ExperimentAssignmentService); + const resolveSegmentsSpy = experimentAssignmentService.resolveSegmentsForEntities as jest.Mock; + const precomputed = module.get(PrecomputedSegmentService); + + service.cacheService.wrap = jest.fn().mockResolvedValue([fastFlag]); + (precomputed.getPrecomputedSets as jest.Mock).mockResolvedValue(new Map()); // no row -> fallback + resolveSegmentsSpy.mockResolvedValue([{}, {}]); + + await service.getKeys(userDoc, 'context1', logger); + + expect(resolveSegmentsSpy).toHaveBeenCalledTimes(1); + }); + }); + + describe('precomputed recompute + seed triggers', () => { + it('seeds an empty precomputed row in-transaction when a flag is created', async () => { + const precomputed = module.get(PrecomputedSegmentService); + flagRepo.insertFeatureFlag = jest.fn().mockResolvedValue([mockFlag1]); + + await service.create(mockFlag2, mockUser1, logger); + + expect(precomputed.seedEmptyRowForFlag).toHaveBeenCalledWith(mockFlag1.id, expect.anything()); + }); + + it('recomputes the affected flag after addList (standalone, owns the transaction)', async () => { + const precomputed = module.get(PrecomputedSegmentService); + + await service.addList([mockList], LIST_FILTER_MODE.INCLUSION, mockUser1, logger); + + expect(precomputed.recomputeForFlag).toHaveBeenCalledWith(mockList.id, logger); + }); + + it('recomputes imported flags after the import transaction commits', async () => { + const precomputed = module.get(PrecomputedSegmentService); + + await service.importFeatureFlags( + [{ fileName: 'import.json', fileContent: JSON.stringify(mockFlag4) }], + mockUser1, + logger + ); + + expect(precomputed.recomputeForFlag).toHaveBeenCalled(); + }); + }); }); diff --git a/packages/backend/test/unit/services/PrecomputedSegmentService.test.ts b/packages/backend/test/unit/services/PrecomputedSegmentService.test.ts new file mode 100644 index 0000000000..21ebedba94 --- /dev/null +++ b/packages/backend/test/unit/services/PrecomputedSegmentService.test.ts @@ -0,0 +1,209 @@ +import { PrecomputedSegmentService } from '../../../src/api/services/PrecomputedSegmentService'; +import { UpgradeLogger } from '../../../src/lib/logger/UpgradeLogger'; +import { CACHE_PREFIX } from 'upgrade_types'; +import { configureLogger } from '../../utils/logger'; + +const logger = new UpgradeLogger(); + +// Build a segment-repository query-builder mock whose getMany() returns the fixtures +// matching the ids captured from the `.where('segment.id IN (:...ids)', { ids })` call. +// flattenSegmentMembers calls createQueryBuilder() once per recursion level, so each call +// returns a fresh builder that resolves against the shared fixture map. +function makeSegmentRepoMock(fixtures: Record) { + const createQueryBuilder = jest.fn(() => { + let capturedIds: string[] = []; + const qb: any = { + leftJoinAndSelect: jest.fn().mockReturnThis(), + where: jest.fn((_sql: string, params: { ids: string[] }) => { + capturedIds = params.ids; + return qb; + }), + getMany: jest.fn(() => Promise.resolve(capturedIds.map((id) => fixtures[id]).filter(Boolean))), + }; + return qb; + }); + return { createQueryBuilder, findParentSegmentIds: jest.fn().mockResolvedValue([]) }; +} + +describe('PrecomputedSegmentService', () => { + beforeAll(() => { + configureLogger(); + }); + + let precomputedSegmentRepository: any; + let featureFlagSegmentInclusionRepository: any; + let featureFlagSegmentExclusionRepository: any; + let featureFlagRepository: any; + let segmentRepository: any; + let cacheService: any; + let service: PrecomputedSegmentService; + + beforeEach(() => { + precomputedSegmentRepository = { + upsertByFlagId: jest.fn().mockResolvedValue(undefined), + findByFlagIds: jest.fn().mockResolvedValue([]), + find: jest.fn().mockResolvedValue([]), + }; + featureFlagSegmentInclusionRepository = { find: jest.fn().mockResolvedValue([]) }; + featureFlagSegmentExclusionRepository = { find: jest.fn().mockResolvedValue([]) }; + featureFlagRepository = { find: jest.fn().mockResolvedValue([]) }; + segmentRepository = makeSegmentRepoMock({}); + cacheService = { + delCache: jest.fn().mockResolvedValue(undefined), + wrapFunction: jest.fn(), + }; + + service = new PrecomputedSegmentService( + precomputedSegmentRepository, + featureFlagSegmentInclusionRepository, + featureFlagSegmentExclusionRepository, + featureFlagRepository, + segmentRepository, + cacheService + ); + }); + + describe('recomputeForFlag', () => { + it('flattens individual + group members, recurses into sub-segments, dedupes, and upserts', async () => { + // segA -> members u1, g1, and a sub-segment segChild (-> u2). segB (exclusion) -> u3. + segmentRepository = makeSegmentRepoMock({ + segA: { + id: 'segA', + individualForSegment: [{ userId: 'u1' }], + groupForSegment: [{ groupId: 'g1' }], + subSegments: [{ id: 'segChild' }], + }, + segChild: { + id: 'segChild', + individualForSegment: [{ userId: 'u2' }], + groupForSegment: [], + subSegments: [], + }, + segB: { + id: 'segB', + individualForSegment: [{ userId: 'u3' }], + groupForSegment: [], + subSegments: [], + }, + }); + featureFlagSegmentInclusionRepository.find = jest.fn().mockResolvedValue([{ segment: { id: 'segA' } }]); + featureFlagSegmentExclusionRepository.find = jest.fn().mockResolvedValue([{ segment: { id: 'segB' } }]); + + service = new PrecomputedSegmentService( + precomputedSegmentRepository, + featureFlagSegmentInclusionRepository, + featureFlagSegmentExclusionRepository, + featureFlagRepository, + segmentRepository, + cacheService + ); + + await service.recomputeForFlag('flag1', logger); + + expect(precomputedSegmentRepository.upsertByFlagId).toHaveBeenCalledTimes(1); + const [flagId, inclusionIds, exclusionIds] = precomputedSegmentRepository.upsertByFlagId.mock.calls[0]; + expect(flagId).toEqual('flag1'); + // recursive sub-segment member u2 must be included alongside the direct members + expect(inclusionIds.sort()).toEqual(['g1', 'u1', 'u2']); + expect(exclusionIds).toEqual(['u3']); + // only enabled lists are queried + expect(featureFlagSegmentInclusionRepository.find).toHaveBeenCalledWith( + expect.objectContaining({ where: { featureFlag: { id: 'flag1' }, enabled: true } }) + ); + // cache for this flag is invalidated + expect(cacheService.delCache).toHaveBeenCalledWith(CACHE_PREFIX.PRECOMPUTED_SEGMENT_KEY_PREFIX + 'flag1'); + }); + + it('produces empty arrays when the flag has no enabled lists', async () => { + await service.recomputeForFlag('flag-empty', logger); + + expect(precomputedSegmentRepository.upsertByFlagId).toHaveBeenCalledWith('flag-empty', [], []); + }); + }); + + describe('getAffectedFlagIds (ancestor walk)', () => { + it('includes flags that reference an ANCESTOR (parent) segment of the edited segment', async () => { + // A flag references segParent; segChild is a sub-segment of segParent. Editing segChild + // must mark the flag referencing segParent as affected. + featureFlagSegmentInclusionRepository.find = jest.fn(({ where }: any) => + Promise.resolve(where.segment.id === 'segParent' ? [{ featureFlag: { id: 'flagP' } }] : []) + ); + featureFlagSegmentExclusionRepository.find = jest.fn().mockResolvedValue([]); + segmentRepository.findParentSegmentIds = jest.fn((id: string) => + Promise.resolve(id === 'segChild' ? ['segParent'] : []) + ); + + const affected = await service.getAffectedFlagIds('segChild'); + + expect(affected).toEqual(['flagP']); + expect(segmentRepository.findParentSegmentIds).toHaveBeenCalledWith('segChild'); + }); + + it('does not infinitely recurse on a segment cycle', async () => { + featureFlagSegmentInclusionRepository.find = jest.fn().mockResolvedValue([]); + featureFlagSegmentExclusionRepository.find = jest.fn().mockResolvedValue([]); + // segX <-> segY reference each other as parents + segmentRepository.findParentSegmentIds = jest.fn((id: string) => + Promise.resolve(id === 'segX' ? ['segY'] : ['segX']) + ); + + const affected = await service.getAffectedFlagIds('segX'); + + expect(affected).toEqual([]); + }); + }); + + describe('getPrecomputedSets', () => { + it('returns an empty map for an empty flag id list without hitting the cache', async () => { + const result = await service.getPrecomputedSets([]); + + expect(result.size).toEqual(0); + expect(cacheService.wrapFunction).not.toHaveBeenCalled(); + }); + + it('maps flag ids to rows positionally and skips missing (null) rows', async () => { + const rowA = { featureFlagId: 'fa', inclusionIds: ['u1'], exclusionIds: [] }; + cacheService.wrapFunction = jest.fn().mockResolvedValue([rowA, null]); + + const result = await service.getPrecomputedSets(['fa', 'fb']); + + expect(result.get('fa')).toEqual(rowA); + expect(result.has('fb')).toEqual(false); + }); + }); + + describe('seedEmptyRowForFlag', () => { + it('inserts an empty row through the provided transaction manager (orIgnore)', async () => { + const execute = jest.fn().mockResolvedValue(undefined); + const values = jest.fn().mockReturnThis(); + const orIgnore = jest.fn().mockReturnThis(); + const qb: any = { + insert: jest.fn().mockReturnThis(), + into: jest.fn().mockReturnThis(), + values, + orIgnore, + execute, + }; + const manager: any = { createQueryBuilder: jest.fn(() => qb) }; + + await service.seedEmptyRowForFlag('flag-new', manager); + + expect(values).toHaveBeenCalledWith({ featureFlagId: 'flag-new', inclusionIds: [], exclusionIds: [] }); + expect(orIgnore).toHaveBeenCalled(); + expect(execute).toHaveBeenCalled(); + }); + }); + + describe('backfillMissingFlags', () => { + it('recomputes only flags that have no precomputed row yet', async () => { + featureFlagRepository.find = jest.fn().mockResolvedValue([{ id: 'f1' }, { id: 'f2' }]); + precomputedSegmentRepository.find = jest.fn().mockResolvedValue([{ featureFlagId: 'f1' }]); + const recomputeSpy = jest.spyOn(service, 'recomputeForFlag').mockResolvedValue(undefined); + + await service.backfillMissingFlags(logger); + + expect(recomputeSpy).toHaveBeenCalledTimes(1); + expect(recomputeSpy).toHaveBeenCalledWith('f2', logger); + }); + }); +}); diff --git a/packages/backend/test/unit/services/SegmentService.test.ts b/packages/backend/test/unit/services/SegmentService.test.ts index f3042d969e..0e7bdc9927 100644 --- a/packages/backend/test/unit/services/SegmentService.test.ts +++ b/packages/backend/test/unit/services/SegmentService.test.ts @@ -12,6 +12,7 @@ import { ExperimentSegmentInclusionRepository } from '../../../src/api/repositor import { FeatureFlagSegmentExclusionRepository } from '../../../src/api/repositories/FeatureFlagSegmentExclusionRepository'; import { FeatureFlagSegmentInclusionRepository } from '../../../src/api/repositories/FeatureFlagSegmentInclusionRepository'; import { CacheService } from '../../../src/api/services/CacheService'; +import { PrecomputedSegmentService } from '../../../src/api/services/PrecomputedSegmentService'; import { ListInputValidator, SegmentFile, @@ -195,6 +196,16 @@ describe('Segment Service Testing', () => { FeatureFlagSegmentInclusionRepository, CacheService, SegmentRepository, + { + provide: PrecomputedSegmentService, + useValue: { + scheduleRecomputeForSegment: jest.fn(), + recomputeForFlag: jest.fn().mockResolvedValue(undefined), + getAffectedFlagIds: jest.fn().mockResolvedValue([]), + seedEmptyRowForFlag: jest.fn().mockResolvedValue(undefined), + getPrecomputedSets: jest.fn().mockResolvedValue(new Map()), + }, + }, { provide: getDataSourceToken('default'), useValue: dataSource, @@ -736,6 +747,36 @@ describe('Segment Service Testing', () => { }).rejects.toThrow(err); }); + describe('precomputed segment recompute triggers', () => { + it('collects affected flags before deleting and recomputes them after the delete commits', async () => { + const precomputed = module.get(PrecomputedSegmentService); + (precomputed.getAffectedFlagIds as jest.Mock).mockResolvedValue(['flagA']); + + await service.deleteSegment(seg1.id, logger); + + expect(precomputed.getAffectedFlagIds).toHaveBeenCalledWith(seg1.id); + expect(precomputed.recomputeForFlag).toHaveBeenCalledWith('flagA', logger); + }); + + it('schedules a recompute when a list is added to a segment', async () => { + const precomputed = module.get(PrecomputedSegmentService); + service.upsertSegmentInPipeline = jest.fn().mockResolvedValue(segValSegment); + + await service.addList(listVal, logger); + + expect(precomputed.scheduleRecomputeForSegment).toHaveBeenCalled(); + }); + + it('schedules a recompute when a list is deleted from a segment', async () => { + const precomputed = module.get(PrecomputedSegmentService); + service.getSegmentById = jest.fn().mockResolvedValue(newSeg); + + await service.deleteList(newList.id, newSeg.id, logger); + + expect(precomputed.scheduleRecomputeForSegment).toHaveBeenCalled(); + }); + }); + it('should find all paginated segments with search string all', async () => { const res = [ { From 6d8c41a43dbc2cd522d0a84286f82b8701d8e9c7 Mon Sep 17 00:00:00 2001 From: Ben Blanchard Date: Tue, 30 Jun 2026 15:35:52 -0400 Subject: [PATCH 05/16] Hotfix/cycle within subject conditions by experiment (#3201) * use the aggregated count for all repeated enrollments to determine round-robin cycle * get results for each experiment, rather than all * explicit type Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix query * add unit test for single rotation counter across DPs --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../RepeatedEnrollmentRepository.ts | 10 ++--- .../services/ExperimentAssignmentService.ts | 15 +++---- .../ExperimentAssignmentService.test.ts | 41 +++++++++++++++++++ 3 files changed, 51 insertions(+), 15 deletions(-) diff --git a/packages/backend/src/api/repositories/RepeatedEnrollmentRepository.ts b/packages/backend/src/api/repositories/RepeatedEnrollmentRepository.ts index afde9a0e1b..e5b7d19c8f 100644 --- a/packages/backend/src/api/repositories/RepeatedEnrollmentRepository.ts +++ b/packages/backend/src/api/repositories/RepeatedEnrollmentRepository.ts @@ -6,23 +6,23 @@ import repositoryError from './utils/repositoryError'; export interface RepeatedEnrollmentDataCount { userId: string; - decisionPointId: string; + experimentId: string; count: number; } @EntityRepository(RepeatedEnrollment) export class RepeatedEnrollmentRepository extends Repository { public async getRepeatedEnrollmentCount( userId: string, - decisionPointsIds: string[], + experimentIds: string[], logger: UpgradeLogger ): Promise { const result = await this.createQueryBuilder('repeatedEnrollment') - .select(['ie.userId as "userId"', 'ie.partitionId as "decisionPointId"']) + .select(['ie.userId as "userId"', 'ie.experimentId as "experimentId"']) .addSelect('COUNT(*) as count') .leftJoin('repeatedEnrollment.individualEnrollment', 'ie') .where('ie.userId = :userId', { userId }) - .andWhere('ie.partitionId IN (:...decisionPointsIds)', { decisionPointsIds }) - .groupBy('ie.userId , ie.partitionId , ie.id') + .andWhere('ie.experimentId IN (:...experimentIds)', { experimentIds }) + .groupBy('ie.userId, ie.experimentId') .getRawMany() .catch((errorMsg: any) => { const errorMsgString = repositoryError( diff --git a/packages/backend/src/api/services/ExperimentAssignmentService.ts b/packages/backend/src/api/services/ExperimentAssignmentService.ts index a32bb3b48b..b15f2a5701 100644 --- a/packages/backend/src/api/services/ExperimentAssignmentService.ts +++ b/packages/backend/src/api/services/ExperimentAssignmentService.ts @@ -432,13 +432,9 @@ export class ExperimentAssignmentService { (experiment) => experiment.assignmentUnit === ASSIGNMENT_UNIT.WITHIN_SUBJECTS ); - const allWithinSubjectDecisionPoints = filteredWithinSubjectExperiments - .map((experiment) => this.getActiveDecisionPoints(experiment)) - .flat(); - repeatedEnrollmentCounts = await this.repeatedEnrollmentRepository.getRepeatedEnrollmentCount( userId, - allWithinSubjectDecisionPoints.map((dp) => dp.id), + filteredWithinSubjectExperiments.map((experiment) => experiment.id), logger ); } @@ -454,7 +450,7 @@ export class ExperimentAssignmentService { conditionPayloads, type, factors, - repeatedEnrollmentCounts || [], + repeatedEnrollmentCounts, logger ); return [...accumulator, ...decisionPoints]; @@ -894,7 +890,7 @@ export class ExperimentAssignmentService { conditionPayloads: ConditionPayloadDTO[], type: EXPERIMENT_TYPE, factors: FactorDTO[], - repeatedEnrollmentCounts: { userId: string; decisionPointId: string; count: number }[], + repeatedEnrollmentCounts: RepeatedEnrollmentDataCount[], logger: UpgradeLogger ): IExperimentAssignmentv5[] { return experiment.partitions @@ -929,9 +925,8 @@ export class ExperimentAssignmentService { if (experiment.assignmentUnit === ASSIGNMENT_UNIT.WITHIN_SUBJECTS) { const count = - repeatedEnrollmentCounts.find( - (repeatedEnrollment) => repeatedEnrollment.decisionPointId === decisionPoint.id - )?.count || 0; + repeatedEnrollmentCounts?.find((repeatedEnrollment) => repeatedEnrollment.experimentId === experiment.id) + ?.count || 0; return withInSubjectType(experiment, conditionPayloads, decisionPoint, factors, userId, count); } else { const experimentId = experiment.id; diff --git a/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts b/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts index 28588d8d21..7f6781b6e7 100644 --- a/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts +++ b/packages/backend/test/unit/services/ExperimentAssignmentService.test.ts @@ -409,6 +409,47 @@ describe('Experiment Assignment Service Test', () => { expect(result[0].assignedCondition).toMatchObject(cond); }); + it('should use a single shared rotation counter across all decision points in a within-subject experiment', async () => { + const context = 'context'; + const userDoc = { id: 'user123', group: { schoolId: ['school1'] }, workingGroup: {} }; + const exp = structuredClone(simpleWithinSubjectOrderedRoundRobinExperiment); + + // Add a second decision point to the experiment + const secondPartition = { + ...exp.partitions[0], + id: 'dp-2-id', + twoCharacterId: 'W2', + site: 'CurriculumSequence', + target: 'W2', + }; + exp.partitions = [...exp.partitions, secondPartition]; + + // Simulate the user has already been through the rotation once (count = 1) + // For ORDERED_ROUND_ROBIN with 2 conditions and count=1, the second condition should be first + const repeatedEnrollmentCount = 1; + testedModule.repeatedEnrollmentRepository = { + getRepeatedEnrollmentCount: sandbox + .stub() + .resolves([{ userId: userDoc.id, experimentId: exp.id, count: repeatedEnrollmentCount }]), + }; + + testedModule.experimentService.getCachedValidExperiments = sandbox.stub().resolves([exp]); + testedModule.experimentUserService = { getOriginalUserDoc: sandbox.stub().resolves(userDoc) }; + + const result = await testedModule.getAllExperimentConditions(userDoc, context, loggerMock); + + // Both decision points should be returned + expect(result.length).toEqual(2); + + // Both DPs must have the same assigned condition order — they share one rotation counter + expect(result[0].assignedCondition[0].conditionCode).toEqual(result[1].assignedCondition[0].conditionCode); + expect(result[0].assignedCondition[1].conditionCode).toEqual(result[1].assignedCondition[1].conditionCode); + + // With count=1, the rotation should have advanced past position 0 — condition at index 1 should now be first + expect(result[0].assignedCondition[0].conditionCode).toEqual(exp.conditions[1].conditionCode); + expect(result[0].assignedCondition[1].conditionCode).toEqual(exp.conditions[0].conditionCode); + }); + it('should return the assigned condition for a simple group experiment', async () => { const context = 'context'; const userDoc = { id: 'user123', group: { 'add-group1': ['school1'] }, workingGroup: { 'add-group1': 'school1' } }; From e0bb14b0e4ea9a992e7b74785584a0c95435ca04 Mon Sep 17 00:00:00 2001 From: doswalt Date: Wed, 1 Jul 2026 13:24:12 -0400 Subject: [PATCH 06/16] name change of precomputed segment table for feature flags and a few smaller improvements --- ...nt.ts => FeatureFlagPrecomputedSegment.ts} | 2 +- ...eatureFlagPrecomputedSegmentRepository.ts} | 13 ++++--- ...> FeatureFlagPrecomputedSegmentService.ts} | 38 ++++++++++--------- .../src/api/services/FeatureFlagService.ts | 30 +++++++-------- .../src/api/services/SegmentService.ts | 20 ++++++---- packages/backend/src/app.ts | 4 +- ...26517264-featureFlagPrecomputedSegment.ts} | 12 +++--- .../backfillFeatureFlagPrecomputedSegments.ts | 10 +++++ .../init/seed/backfillPrecomputedSegments.ts | 8 ---- ...tureFlagPrecomputedSegmentService.test.ts} | 12 +++--- .../unit/services/FeatureFlagService.test.ts | 16 ++++---- .../test/unit/services/SegmentService.test.ts | 26 ++++++++++--- packages/types/src/Experiment/enums.ts | 2 +- 13 files changed, 110 insertions(+), 83 deletions(-) rename packages/backend/src/api/models/{PrecomputedSegment.ts => FeatureFlagPrecomputedSegment.ts} (89%) rename packages/backend/src/api/repositories/{PrecomputedSegmentRepository.ts => FeatureFlagPrecomputedSegmentRepository.ts} (53%) rename packages/backend/src/api/services/{PrecomputedSegmentService.ts => FeatureFlagPrecomputedSegmentService.ts} (80%) rename packages/backend/src/database/migrations/{1779500000000-precomputedSegment.ts => 1782926517264-featureFlagPrecomputedSegment.ts} (59%) create mode 100644 packages/backend/src/init/seed/backfillFeatureFlagPrecomputedSegments.ts delete mode 100644 packages/backend/src/init/seed/backfillPrecomputedSegments.ts rename packages/backend/test/unit/services/{PrecomputedSegmentService.test.ts => FeatureFlagPrecomputedSegmentService.test.ts} (95%) diff --git a/packages/backend/src/api/models/PrecomputedSegment.ts b/packages/backend/src/api/models/FeatureFlagPrecomputedSegment.ts similarity index 89% rename from packages/backend/src/api/models/PrecomputedSegment.ts rename to packages/backend/src/api/models/FeatureFlagPrecomputedSegment.ts index e14be00605..8d2e364549 100644 --- a/packages/backend/src/api/models/PrecomputedSegment.ts +++ b/packages/backend/src/api/models/FeatureFlagPrecomputedSegment.ts @@ -3,7 +3,7 @@ import { BaseModel } from './base/BaseModel'; import { FeatureFlag } from './FeatureFlag'; @Entity() -export class PrecomputedSegment extends BaseModel { +export class FeatureFlagPrecomputedSegment extends BaseModel { @PrimaryColumn('uuid') public featureFlagId: string; diff --git a/packages/backend/src/api/repositories/PrecomputedSegmentRepository.ts b/packages/backend/src/api/repositories/FeatureFlagPrecomputedSegmentRepository.ts similarity index 53% rename from packages/backend/src/api/repositories/PrecomputedSegmentRepository.ts rename to packages/backend/src/api/repositories/FeatureFlagPrecomputedSegmentRepository.ts index 9ac3d3cd88..8d11152976 100644 --- a/packages/backend/src/api/repositories/PrecomputedSegmentRepository.ts +++ b/packages/backend/src/api/repositories/FeatureFlagPrecomputedSegmentRepository.ts @@ -1,21 +1,22 @@ import { Repository } from 'typeorm'; import { EntityRepository } from '../../typeorm-typedi-extensions'; -import { PrecomputedSegment } from '../models/PrecomputedSegment'; +import { FeatureFlagPrecomputedSegment } from '../models/FeatureFlagPrecomputedSegment'; -@EntityRepository(PrecomputedSegment) -export class PrecomputedSegmentRepository extends Repository { +@EntityRepository(FeatureFlagPrecomputedSegment) +export class FeatureFlagPrecomputedSegmentRepository extends Repository { public async upsertByFlagId(flagId: string, inclusionIds: string[], exclusionIds: string[]): Promise { await this.createQueryBuilder() .insert() - .into(PrecomputedSegment) + .into(FeatureFlagPrecomputedSegment) .values({ featureFlagId: flagId, inclusionIds, exclusionIds }) .orUpdate(['inclusionIds', 'exclusionIds', 'updatedAt'], ['featureFlagId']) .execute(); } - public async findByFlagIds(flagIds: string[]): Promise<(PrecomputedSegment | null)[]> { + public async findByFlagIds(flagIds: string[]): Promise<(FeatureFlagPrecomputedSegment | null)[]> { if (!flagIds.length) return []; const rows = await this.createQueryBuilder('ps').where('ps.featureFlagId IN (:...ids)', { ids: flagIds }).getMany(); - return flagIds.map((id) => rows.find((r) => r.featureFlagId === id) ?? null); + const rowsByFlagId = new Map(rows.map((r) => [r.featureFlagId, r])); + return flagIds.map((id) => rowsByFlagId.get(id) ?? null); } } diff --git a/packages/backend/src/api/services/PrecomputedSegmentService.ts b/packages/backend/src/api/services/FeatureFlagPrecomputedSegmentService.ts similarity index 80% rename from packages/backend/src/api/services/PrecomputedSegmentService.ts rename to packages/backend/src/api/services/FeatureFlagPrecomputedSegmentService.ts index 5545f0f13b..23d0b30cc7 100644 --- a/packages/backend/src/api/services/PrecomputedSegmentService.ts +++ b/packages/backend/src/api/services/FeatureFlagPrecomputedSegmentService.ts @@ -1,20 +1,20 @@ import { Service } from 'typedi'; import { InjectRepository } from '../../typeorm-typedi-extensions'; -import { PrecomputedSegmentRepository } from '../repositories/PrecomputedSegmentRepository'; +import { FeatureFlagPrecomputedSegmentRepository } from '../repositories/FeatureFlagPrecomputedSegmentRepository'; import { FeatureFlagSegmentInclusionRepository } from '../repositories/FeatureFlagSegmentInclusionRepository'; import { FeatureFlagSegmentExclusionRepository } from '../repositories/FeatureFlagSegmentExclusionRepository'; import { FeatureFlagRepository } from '../repositories/FeatureFlagRepository'; import { SegmentRepository } from '../repositories/SegmentRepository'; -import { PrecomputedSegment } from '../models/PrecomputedSegment'; +import { FeatureFlagPrecomputedSegment } from '../models/FeatureFlagPrecomputedSegment'; import { CacheService } from './CacheService'; import { CACHE_PREFIX } from 'upgrade_types'; import { UpgradeLogger } from '../../lib/logger/UpgradeLogger'; import { EntityManager } from 'typeorm'; @Service() -export class PrecomputedSegmentService { +export class FeatureFlagPrecomputedSegmentService { constructor( - @InjectRepository() private precomputedSegmentRepository: PrecomputedSegmentRepository, + @InjectRepository() private precomputedSegmentRepository: FeatureFlagPrecomputedSegmentRepository, @InjectRepository() private featureFlagSegmentInclusionRepository: FeatureFlagSegmentInclusionRepository, @InjectRepository() private featureFlagSegmentExclusionRepository: FeatureFlagSegmentExclusionRepository, @InjectRepository() private featureFlagRepository: FeatureFlagRepository, @@ -48,11 +48,11 @@ export class PrecomputedSegmentService { [...new Set(exclusionIds)] ); - await this.cacheService.delCache(CACHE_PREFIX.PRECOMPUTED_SEGMENT_KEY_PREFIX + flagId); - logger.info({ message: `Recomputed precomputed_segment for flag ${flagId}` }); + await this.cacheService.delCache(CACHE_PREFIX.FEATURE_FLAG_PRECOMPUTED_SEGMENT_KEY_PREFIX + flagId); + logger.info({ message: `Recomputed feature_flag_precomputed_segment for flag ${flagId}` }); } - // Seed an empty precomputed_segment row for a brand-new flag, inside the flag's own + // Seed an empty feature_flag_precomputed_segment row for a brand-new flag, inside the flag's own // creation transaction so the row is atomic with the flag insert. A new flag has no // segment lists, so empty arrays are the correct initial state. `orIgnore` keeps this a // no-op if a row somehow already exists. Seeding here guarantees getPrecomputedSets never @@ -61,7 +61,7 @@ export class PrecomputedSegmentService { await manager .createQueryBuilder() .insert() - .into(PrecomputedSegment) + .into(FeatureFlagPrecomputedSegment) .values({ featureFlagId: flagId, inclusionIds: [], exclusionIds: [] }) .orIgnore() .execute(); @@ -74,16 +74,18 @@ export class PrecomputedSegmentService { .catch((err) => logger.error({ message: `Error in scheduleRecomputeForSegment: ${err}` })); } - public async getPrecomputedSets(flagIds: string[]): Promise> { + public async getPrecomputedSets(flagIds: string[]): Promise> { if (!flagIds.length) return new Map(); - const results = await this.cacheService.wrapFunction(CACHE_PREFIX.PRECOMPUTED_SEGMENT_KEY_PREFIX, flagIds, () => - this.precomputedSegmentRepository.findByFlagIds(flagIds) + const results = await this.cacheService.wrapFunction( + CACHE_PREFIX.FEATURE_FLAG_PRECOMPUTED_SEGMENT_KEY_PREFIX, + flagIds, + () => this.precomputedSegmentRepository.findByFlagIds(flagIds) ); - const map = new Map(); + const map = new Map(); flagIds.forEach((id, i) => { - if (results[i]) map.set(id, results[i] as PrecomputedSegment); + if (results[i]) map.set(id, results[i] as FeatureFlagPrecomputedSegment); }); return map; } @@ -95,13 +97,13 @@ export class PrecomputedSegmentService { try { await this.recomputeForFlag(flag.id, logger); } catch (err) { - logger.error({ message: `Failed to recompute precomputed_segment for flag ${flag.id}: ${err}` }); + logger.error({ message: `Failed to recompute feature_flag_precomputed_segment for flag ${flag.id}: ${err}` }); } } logger.info({ message: `Backfill complete: recomputed ${flags.length} flags` }); } - // Backfill only flags that have no precomputed_segment row yet — safe to run every startup + // Backfill only flags that have no feature_flag_precomputed_segment row yet — safe to run every startup public async backfillMissingFlags(logger: UpgradeLogger): Promise { const [allFlags, existingRows] = await Promise.all([ this.featureFlagRepository.find({ select: ['id'] }), @@ -112,7 +114,7 @@ export class PrecomputedSegmentService { const missingFlags = allFlags.filter((f) => !existingFlagIds.has(f.id)); if (!missingFlags.length) { - logger.info({ message: 'precomputed_segment backfill: all flags already have rows, nothing to do' }); + logger.info({ message: 'feature_flag_precomputed_segment backfill: all flags already have rows, nothing to do' }); return; } @@ -120,11 +122,11 @@ export class PrecomputedSegmentService { try { await this.recomputeForFlag(flag.id, logger); } catch (err) { - logger.error({ message: `Failed to backfill precomputed_segment for flag ${flag.id}: ${err}` }); + logger.error({ message: `Failed to backfill feature_flag_precomputed_segment for flag ${flag.id}: ${err}` }); } } logger.info({ - message: `precomputed_segment backfill complete: computed ${missingFlags.length} of ${allFlags.length} flags`, + message: `feature_flag_precomputed_segment backfill complete: computed ${missingFlags.length} of ${allFlags.length} flags`, }); } diff --git a/packages/backend/src/api/services/FeatureFlagService.ts b/packages/backend/src/api/services/FeatureFlagService.ts index 8568ac4d1e..3af1293c61 100644 --- a/packages/backend/src/api/services/FeatureFlagService.ts +++ b/packages/backend/src/api/services/FeatureFlagService.ts @@ -54,7 +54,7 @@ import { SegmentRepository } from '../repositories/SegmentRepository'; import { ExperimentAuditLog } from '../models/ExperimentAuditLog'; import { NotFoundException } from '@nestjs/common/exceptions'; import { CacheService } from './CacheService'; -import { PrecomputedSegmentService } from './PrecomputedSegmentService'; +import { FeatureFlagPrecomputedSegmentService } from './FeatureFlagPrecomputedSegmentService'; import { SegmentFile, SegmentInputValidator } from '../controllers/validators/SegmentInputValidator'; import dayjs from 'dayjs'; import { getDateRangeNames } from '../repositories/utils/dateQuery'; @@ -72,7 +72,7 @@ export class FeatureFlagService { public experimentAssignmentService: ExperimentAssignmentService, public segmentService: SegmentService, public cacheService: CacheService, - public precomputedSegmentService: PrecomputedSegmentService + public featureFlagPrecomputedSegmentService: FeatureFlagPrecomputedSegmentService ) {} public find(logger: UpgradeLogger): Promise { @@ -411,10 +411,10 @@ export class FeatureFlagService { }; await this.experimentAuditLogRepository.saveRawJson(LOG_TYPE.FEATURE_FLAG_CREATED, createAuditLogData, user); - // Seed an empty precomputed_segment row in the same transaction so the new flag always + // Seed an empty feature_flag_precomputed_segment row in the same transaction so the new flag always // has a row (no segment lists yet => empty arrays). This keeps the assignment read path // off the on-the-fly fallback for the common case and keeps the getKeys cache effective. - await this.precomputedSegmentService.seedEmptyRowForFlag(featureFlagDoc.id, manager); + await this.featureFlagPrecomputedSegmentService.seedEmptyRowForFlag(featureFlagDoc.id, manager); return featureFlagDoc; }; @@ -534,7 +534,7 @@ export class FeatureFlagService { const deleted = await this.segmentService.deleteSegment(segmentId, logger); if (flagId) { - await this.precomputedSegmentService.recomputeForFlag(flagId, logger); + await this.featureFlagPrecomputedSegmentService.recomputeForFlag(flagId, logger); } return deleted; @@ -687,7 +687,7 @@ export class FeatureFlagService { if (transactionalEntityManager) { // The caller owns the outer transaction. We must NOT recompute here: recomputeForFlag // reads through its own repositories and cannot see this transaction's uncommitted writes, - // so it would persist an empty/stale precomputed_segment row that never self-heals. The + // so it would persist an empty/stale feature_flag_precomputed_segment row that never self-heals. The // caller is responsible for calling recomputeForFlag after its transaction commits. result = await executeTransaction(transactionalEntityManager); } else { @@ -698,7 +698,7 @@ export class FeatureFlagService { // Recompute precomputed sets for each affected flag after the transaction commits const affectedFlagIds = [...new Set(listsInput.map((l) => l.id))]; await Promise.all( - affectedFlagIds.map((flagId) => this.precomputedSegmentService.recomputeForFlag(flagId, logger)) + affectedFlagIds.map((flagId) => this.featureFlagPrecomputedSegmentService.recomputeForFlag(flagId, logger)) ); } @@ -853,7 +853,7 @@ export class FeatureFlagService { return existingRecord; }); - await this.precomputedSegmentService.recomputeForFlag(listInput.id, logger); + await this.featureFlagPrecomputedSegmentService.recomputeForFlag(listInput.id, logger); return result; } @@ -918,7 +918,7 @@ export class FeatureFlagService { logger: UpgradeLogger ): Promise[]> { const flagIds = featureFlags.map((f) => f.id); - const precomputedMap = await this.precomputedSegmentService.getPrecomputedSets(flagIds); + const precomputedMap = await this.featureFlagPrecomputedSegmentService.getPrecomputedSets(flagIds); // Flatten all group IDs from the user's group map (type is ignored per design decision) const userGroupIds: string[] = experimentUser.group ? Object.values(experimentUser.group).flat() : []; @@ -962,7 +962,7 @@ export class FeatureFlagService { } /** - * Fallback assignment path for flags that have no precomputed_segment row. Resolves segment + * Fallback assignment path for flags that have no feature_flag_precomputed_segment row. Resolves segment * inclusion/exclusion on-the-fly using the same recursive resolution the codebase used before * precomputed segments (and that experiments still use), preserving full group-type matching. * Returns the set of flag IDs the user should be included in. @@ -974,7 +974,7 @@ export class FeatureFlagService { logger: UpgradeLogger ): Promise> { logger.warn({ - message: `featureFlagLevelInclusionExclusion: ${missingFlagIds.length} flag(s) missing a precomputed_segment row; resolving on-the-fly`, + message: `featureFlagLevelInclusionExclusion: ${missingFlagIds.length} flag(s) missing a feature_flag_precomputed_segment row; resolving on-the-fly`, details: { context, missingFlagIds }, }); @@ -1173,8 +1173,8 @@ export class FeatureFlagService { createdFlags.push(createdFlag); // The outer transaction has committed — recompute now (addList skipped it because it ran - // inside the transaction) so the imported enabled lists are reflected in precomputed_segment. - await this.precomputedSegmentService.recomputeForFlag(createdFlag.id, logger); + // inside the transaction) so the imported enabled lists are reflected in feature_flag_precomputed_segment. + await this.featureFlagPrecomputedSegmentService.recomputeForFlag(createdFlag.id, logger); } logger.info({ message: 'Imported feature flags', details: createdFlags }); @@ -1369,8 +1369,8 @@ export class FeatureFlagService { }); // The outer transaction has committed — recompute now (addList skipped it because it ran - // inside the transaction) so the imported lists are reflected in precomputed_segment. - await this.precomputedSegmentService.recomputeForFlag(featureFlagId, logger); + // inside the transaction) so the imported lists are reflected in feature_flag_precomputed_segment. + await this.featureFlagPrecomputedSegmentService.recomputeForFlag(featureFlagId, logger); logger.info({ message: 'Imported feature flags', details: createdLists }); diff --git a/packages/backend/src/api/services/SegmentService.ts b/packages/backend/src/api/services/SegmentService.ts index e98ad1894f..caa3560fad 100644 --- a/packages/backend/src/api/services/SegmentService.ts +++ b/packages/backend/src/api/services/SegmentService.ts @@ -37,7 +37,7 @@ import { FeatureFlagSegmentExclusionRepository } from '../repositories/FeatureFl import { FeatureFlagSegmentInclusionRepository } from '../repositories/FeatureFlagSegmentInclusionRepository'; import { getSegmentData, getSegmentsData } from '../controllers/SegmentController'; import { CacheService } from './CacheService'; -import { PrecomputedSegmentService } from './PrecomputedSegmentService'; +import { FeatureFlagPrecomputedSegmentService } from './FeatureFlagPrecomputedSegmentService'; import { isUUID, validate } from 'class-validator'; import { plainToClass } from 'class-transformer'; import path from 'path'; @@ -83,7 +83,7 @@ export class SegmentService { @InjectRepository() private featureFlagSegmentInclusionRepository: FeatureFlagSegmentInclusionRepository, private cacheService: CacheService, - private precomputedSegmentService: PrecomputedSegmentService + private featureFlagPrecomputedSegmentService: FeatureFlagPrecomputedSegmentService ) {} public async getAllSegments(logger: UpgradeLogger): Promise { @@ -430,7 +430,7 @@ export class SegmentService { return createdSegment; }); - this.precomputedSegmentService.scheduleRecomputeForSegment(parentSegmentId, logger); + this.featureFlagPrecomputedSegmentService.scheduleRecomputeForSegment(parentSegmentId, logger); return createdSegment; } @@ -458,7 +458,7 @@ export class SegmentService { return deletedSegmentResponse; }); - this.precomputedSegmentService.scheduleRecomputeForSegment(parentSegmentId, logger); + this.featureFlagPrecomputedSegmentService.scheduleRecomputeForSegment(parentSegmentId, logger); // reset cache await this.cacheService.resetPrefixCache(CACHE_PREFIX.SEGMENT_KEY_PREFIX); @@ -480,7 +480,7 @@ export class SegmentService { public async deleteSegment(id: string, logger: UpgradeLogger): Promise { logger.info({ message: `Delete segment by id. segmentId: ${id}` }); // Collect affected flags before deletion — join table records are gone after - const affectedFlagIds = await this.precomputedSegmentService.getAffectedFlagIds(id); + const affectedFlagIds = await this.featureFlagPrecomputedSegmentService.getAffectedFlagIds(id); const manager = this.dataSource; const deletedSegment = await manager.transaction(async (transactionalEntityManager) => { @@ -489,7 +489,13 @@ export class SegmentService { // Recompute after the delete transaction has committed so the recompute reads the // post-delete state and stale member IDs are removed (fire-and-forget). - affectedFlagIds.forEach((flagId) => this.precomputedSegmentService.recomputeForFlag(flagId, logger)); + affectedFlagIds.forEach((flagId) => + this.featureFlagPrecomputedSegmentService + .recomputeForFlag(flagId, logger) + .catch((err) => + logger.error({ message: `Error recomputing feature_flag_precomputed_segment for flag ${flagId}: ${err}` }) + ) + ); // reset cache await this.cacheService.resetPrefixCache(CACHE_PREFIX.SEGMENT_KEY_PREFIX); @@ -1039,7 +1045,7 @@ export class SegmentService { // firing this from inside a transaction risks a stale-read race where the fire-and-forget // reads the old enabled value and its upsert overwrites the correct post-commit result. if (!skipScheduleRecompute) { - this.precomputedSegmentService.scheduleRecomputeForSegment(segmentDoc.id, logger); + this.featureFlagPrecomputedSegmentService.scheduleRecomputeForSegment(segmentDoc.id, logger); } return transactionalEntityManager diff --git a/packages/backend/src/app.ts b/packages/backend/src/app.ts index 133ef132dd..7e10ddd38b 100644 --- a/packages/backend/src/app.ts +++ b/packages/backend/src/app.ts @@ -19,7 +19,7 @@ import { enableMetricFiltering } from './init/seed/EnableMetricFiltering'; import { InitMetrics } from './init/seed/initMetrics'; import { banner } from './lib/banner'; import { createGlobalExcludeSegment } from './init/seed/globalExcludeSegment'; -import { backfillPrecomputedSegments } from './init/seed/backfillPrecomputedSegments'; +import { backfillFeatureFlagPrecomputedSegments } from './init/seed/backfillFeatureFlagPrecomputedSegments'; /* * EXPRESS TYPESCRIPT BOILERPLATE @@ -50,5 +50,5 @@ bootstrapMicroframework({ return createGlobalExcludeSegment(logger); }) .then(() => { - return backfillPrecomputedSegments(logger); + return backfillFeatureFlagPrecomputedSegments(logger); }); diff --git a/packages/backend/src/database/migrations/1779500000000-precomputedSegment.ts b/packages/backend/src/database/migrations/1782926517264-featureFlagPrecomputedSegment.ts similarity index 59% rename from packages/backend/src/database/migrations/1779500000000-precomputedSegment.ts rename to packages/backend/src/database/migrations/1782926517264-featureFlagPrecomputedSegment.ts index 8947c78d4a..caa7464983 100644 --- a/packages/backend/src/database/migrations/1779500000000-precomputedSegment.ts +++ b/packages/backend/src/database/migrations/1782926517264-featureFlagPrecomputedSegment.ts @@ -1,25 +1,25 @@ import { MigrationInterface, QueryRunner } from 'typeorm'; -export class PrecomputedSegment1779500000000 implements MigrationInterface { - name = 'PrecomputedSegment1779500000000'; +export class FeatureFlagPrecomputedSegment1782926517264 implements MigrationInterface { + name = 'FeatureFlagPrecomputedSegment1782926517264'; public async up(queryRunner: QueryRunner): Promise { await queryRunner.query(` - CREATE TABLE "precomputed_segment" ( + CREATE TABLE "feature_flag_precomputed_segment" ( "featureFlagId" uuid NOT NULL, "inclusionIds" text[] NOT NULL DEFAULT '{}', "exclusionIds" text[] NOT NULL DEFAULT '{}', "createdAt" TIMESTAMP NOT NULL DEFAULT now(), "updatedAt" TIMESTAMP NOT NULL DEFAULT now(), "versionNumber" integer NOT NULL DEFAULT 1, - CONSTRAINT "PK_precomputed_segment" PRIMARY KEY ("featureFlagId"), - CONSTRAINT "FK_precomputed_segment_feature_flag" + CONSTRAINT "PK_feature_flag_precomputed_segment" PRIMARY KEY ("featureFlagId"), + CONSTRAINT "FK_feature_flag_precomputed_segment_feature_flag" FOREIGN KEY ("featureFlagId") REFERENCES "feature_flag"("id") ON DELETE CASCADE ) `); } public async down(queryRunner: QueryRunner): Promise { - await queryRunner.query(`DROP TABLE "precomputed_segment"`); + await queryRunner.query(`DROP TABLE "feature_flag_precomputed_segment"`); } } diff --git a/packages/backend/src/init/seed/backfillFeatureFlagPrecomputedSegments.ts b/packages/backend/src/init/seed/backfillFeatureFlagPrecomputedSegments.ts new file mode 100644 index 0000000000..e945b775c2 --- /dev/null +++ b/packages/backend/src/init/seed/backfillFeatureFlagPrecomputedSegments.ts @@ -0,0 +1,10 @@ +import { FeatureFlagPrecomputedSegmentService } from '../../api/services/FeatureFlagPrecomputedSegmentService'; +import { UpgradeLogger } from '../../lib/logger/UpgradeLogger'; +import Container from 'typedi'; + +export async function backfillFeatureFlagPrecomputedSegments(logger: UpgradeLogger): Promise { + const featureFlagPrecomputedSegmentService = Container.get( + FeatureFlagPrecomputedSegmentService + ); + await featureFlagPrecomputedSegmentService.backfillMissingFlags(logger); +} diff --git a/packages/backend/src/init/seed/backfillPrecomputedSegments.ts b/packages/backend/src/init/seed/backfillPrecomputedSegments.ts deleted file mode 100644 index 1ab6686ac4..0000000000 --- a/packages/backend/src/init/seed/backfillPrecomputedSegments.ts +++ /dev/null @@ -1,8 +0,0 @@ -import { PrecomputedSegmentService } from '../../api/services/PrecomputedSegmentService'; -import { UpgradeLogger } from '../../lib/logger/UpgradeLogger'; -import Container from 'typedi'; - -export async function backfillPrecomputedSegments(logger: UpgradeLogger): Promise { - const precomputedSegmentService = Container.get(PrecomputedSegmentService); - await precomputedSegmentService.backfillMissingFlags(logger); -} diff --git a/packages/backend/test/unit/services/PrecomputedSegmentService.test.ts b/packages/backend/test/unit/services/FeatureFlagPrecomputedSegmentService.test.ts similarity index 95% rename from packages/backend/test/unit/services/PrecomputedSegmentService.test.ts rename to packages/backend/test/unit/services/FeatureFlagPrecomputedSegmentService.test.ts index 21ebedba94..38bc84ec59 100644 --- a/packages/backend/test/unit/services/PrecomputedSegmentService.test.ts +++ b/packages/backend/test/unit/services/FeatureFlagPrecomputedSegmentService.test.ts @@ -1,4 +1,4 @@ -import { PrecomputedSegmentService } from '../../../src/api/services/PrecomputedSegmentService'; +import { FeatureFlagPrecomputedSegmentService } from '../../../src/api/services/FeatureFlagPrecomputedSegmentService'; import { UpgradeLogger } from '../../../src/lib/logger/UpgradeLogger'; import { CACHE_PREFIX } from 'upgrade_types'; import { configureLogger } from '../../utils/logger'; @@ -25,7 +25,7 @@ function makeSegmentRepoMock(fixtures: Record) { return { createQueryBuilder, findParentSegmentIds: jest.fn().mockResolvedValue([]) }; } -describe('PrecomputedSegmentService', () => { +describe('FeatureFlagPrecomputedSegmentService', () => { beforeAll(() => { configureLogger(); }); @@ -36,7 +36,7 @@ describe('PrecomputedSegmentService', () => { let featureFlagRepository: any; let segmentRepository: any; let cacheService: any; - let service: PrecomputedSegmentService; + let service: FeatureFlagPrecomputedSegmentService; beforeEach(() => { precomputedSegmentRepository = { @@ -53,7 +53,7 @@ describe('PrecomputedSegmentService', () => { wrapFunction: jest.fn(), }; - service = new PrecomputedSegmentService( + service = new FeatureFlagPrecomputedSegmentService( precomputedSegmentRepository, featureFlagSegmentInclusionRepository, featureFlagSegmentExclusionRepository, @@ -89,7 +89,7 @@ describe('PrecomputedSegmentService', () => { featureFlagSegmentInclusionRepository.find = jest.fn().mockResolvedValue([{ segment: { id: 'segA' } }]); featureFlagSegmentExclusionRepository.find = jest.fn().mockResolvedValue([{ segment: { id: 'segB' } }]); - service = new PrecomputedSegmentService( + service = new FeatureFlagPrecomputedSegmentService( precomputedSegmentRepository, featureFlagSegmentInclusionRepository, featureFlagSegmentExclusionRepository, @@ -111,7 +111,7 @@ describe('PrecomputedSegmentService', () => { expect.objectContaining({ where: { featureFlag: { id: 'flag1' }, enabled: true } }) ); // cache for this flag is invalidated - expect(cacheService.delCache).toHaveBeenCalledWith(CACHE_PREFIX.PRECOMPUTED_SEGMENT_KEY_PREFIX + 'flag1'); + expect(cacheService.delCache).toHaveBeenCalledWith(CACHE_PREFIX.FEATURE_FLAG_PRECOMPUTED_SEGMENT_KEY_PREFIX + 'flag1'); }); it('produces empty arrays when the flag has no enabled lists', async () => { diff --git a/packages/backend/test/unit/services/FeatureFlagService.test.ts b/packages/backend/test/unit/services/FeatureFlagService.test.ts index 3847b37613..67aff6af2b 100644 --- a/packages/backend/test/unit/services/FeatureFlagService.test.ts +++ b/packages/backend/test/unit/services/FeatureFlagService.test.ts @@ -28,7 +28,7 @@ import { ExperimentAssignmentService } from '../../../src/api/services/Experimen import { FeatureFlagValidation } from '../../../src/api/controllers/validators/FeatureFlagValidator'; import { FeatureFlagListValidator } from '../../../src/api/controllers/validators/FeatureFlagListValidator'; import { SegmentService } from '../../../src/api/services/SegmentService'; -import { PrecomputedSegmentService } from '../../../src/api/services/PrecomputedSegmentService'; +import { FeatureFlagPrecomputedSegmentService } from '../../../src/api/services/FeatureFlagPrecomputedSegmentService'; import { FeatureFlagSegmentExclusionRepository } from '../../../src/api/repositories/FeatureFlagSegmentExclusionRepository'; import { FeatureFlagSegmentInclusionRepository } from '../../../src/api/repositories/FeatureFlagSegmentInclusionRepository'; import { FeatureFlagExposureRepository } from '../../../src/api/repositories/FeatureFlagExposureRepository'; @@ -205,7 +205,7 @@ describe('Feature Flag Service Testing', () => { }, }, { - provide: PrecomputedSegmentService, + provide: FeatureFlagPrecomputedSegmentService, useValue: { // Empty map by default => every flag is "missing" a precomputed row, so getKeys // routes through the on-the-fly fallback (resolveSegmentsForEntities/inclusionExclusionLogic). @@ -705,7 +705,7 @@ describe('Feature Flag Service Testing', () => { it('uses the precomputed set and skips on-the-fly resolution when a row exists', async () => { const userDoc = { id: 'user123', group: {}, workingGroup: {} } as any; const experimentAssignmentService = module.get(ExperimentAssignmentService); - const precomputed = module.get(PrecomputedSegmentService); + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); service.cacheService.wrap = jest.fn().mockResolvedValue([fastFlag]); (precomputed.getPrecomputedSets as jest.Mock).mockResolvedValue( @@ -722,7 +722,7 @@ describe('Feature Flag Service Testing', () => { it('individual inclusion beats group exclusion on the fast path', async () => { const userDoc = { id: 'user123', group: { classId: ['bad-class'] }, workingGroup: {} } as any; - const precomputed = module.get(PrecomputedSegmentService); + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); service.cacheService.wrap = jest.fn().mockResolvedValue([fastFlag]); (precomputed.getPrecomputedSets as jest.Mock).mockResolvedValue( @@ -738,7 +738,7 @@ describe('Feature Flag Service Testing', () => { const userDoc = { id: 'user123', group: {}, workingGroup: {} } as any; const experimentAssignmentService = module.get(ExperimentAssignmentService); const resolveSegmentsSpy = experimentAssignmentService.resolveSegmentsForEntities as jest.Mock; - const precomputed = module.get(PrecomputedSegmentService); + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); service.cacheService.wrap = jest.fn().mockResolvedValue([fastFlag]); (precomputed.getPrecomputedSets as jest.Mock).mockResolvedValue(new Map()); // no row -> fallback @@ -752,7 +752,7 @@ describe('Feature Flag Service Testing', () => { describe('precomputed recompute + seed triggers', () => { it('seeds an empty precomputed row in-transaction when a flag is created', async () => { - const precomputed = module.get(PrecomputedSegmentService); + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); flagRepo.insertFeatureFlag = jest.fn().mockResolvedValue([mockFlag1]); await service.create(mockFlag2, mockUser1, logger); @@ -761,7 +761,7 @@ describe('Feature Flag Service Testing', () => { }); it('recomputes the affected flag after addList (standalone, owns the transaction)', async () => { - const precomputed = module.get(PrecomputedSegmentService); + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); await service.addList([mockList], LIST_FILTER_MODE.INCLUSION, mockUser1, logger); @@ -769,7 +769,7 @@ describe('Feature Flag Service Testing', () => { }); it('recomputes imported flags after the import transaction commits', async () => { - const precomputed = module.get(PrecomputedSegmentService); + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); await service.importFeatureFlags( [{ fileName: 'import.json', fileContent: JSON.stringify(mockFlag4) }], diff --git a/packages/backend/test/unit/services/SegmentService.test.ts b/packages/backend/test/unit/services/SegmentService.test.ts index 0e7bdc9927..4df30115a4 100644 --- a/packages/backend/test/unit/services/SegmentService.test.ts +++ b/packages/backend/test/unit/services/SegmentService.test.ts @@ -12,7 +12,7 @@ import { ExperimentSegmentInclusionRepository } from '../../../src/api/repositor import { FeatureFlagSegmentExclusionRepository } from '../../../src/api/repositories/FeatureFlagSegmentExclusionRepository'; import { FeatureFlagSegmentInclusionRepository } from '../../../src/api/repositories/FeatureFlagSegmentInclusionRepository'; import { CacheService } from '../../../src/api/services/CacheService'; -import { PrecomputedSegmentService } from '../../../src/api/services/PrecomputedSegmentService'; +import { FeatureFlagPrecomputedSegmentService } from '../../../src/api/services/FeatureFlagPrecomputedSegmentService'; import { ListInputValidator, SegmentFile, @@ -197,7 +197,7 @@ describe('Segment Service Testing', () => { CacheService, SegmentRepository, { - provide: PrecomputedSegmentService, + provide: FeatureFlagPrecomputedSegmentService, useValue: { scheduleRecomputeForSegment: jest.fn(), recomputeForFlag: jest.fn().mockResolvedValue(undefined), @@ -749,7 +749,7 @@ describe('Segment Service Testing', () => { describe('precomputed segment recompute triggers', () => { it('collects affected flags before deleting and recomputes them after the delete commits', async () => { - const precomputed = module.get(PrecomputedSegmentService); + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); (precomputed.getAffectedFlagIds as jest.Mock).mockResolvedValue(['flagA']); await service.deleteSegment(seg1.id, logger); @@ -758,8 +758,24 @@ describe('Segment Service Testing', () => { expect(precomputed.recomputeForFlag).toHaveBeenCalledWith('flagA', logger); }); + it('logs and does not reject deleteSegment when a fire-and-forget recompute fails', async () => { + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); + (precomputed.getAffectedFlagIds as jest.Mock).mockResolvedValue(['flagA']); + (precomputed.recomputeForFlag as jest.Mock).mockRejectedValueOnce(new Error('recompute boom')); + const errorSpy = jest.spyOn(logger, 'error'); + + // deleteSegment must still resolve — the recompute is fire-and-forget + await expect(service.deleteSegment(seg1.id, logger)).resolves.toBeDefined(); + + // let the fire-and-forget .catch settle so the rejection is handled (no unhandled rejection) + await new Promise((resolve) => setImmediate(resolve)); + + expect(errorSpy).toHaveBeenCalledWith(expect.objectContaining({ message: expect.stringContaining('flagA') })); + errorSpy.mockRestore(); + }); + it('schedules a recompute when a list is added to a segment', async () => { - const precomputed = module.get(PrecomputedSegmentService); + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); service.upsertSegmentInPipeline = jest.fn().mockResolvedValue(segValSegment); await service.addList(listVal, logger); @@ -768,7 +784,7 @@ describe('Segment Service Testing', () => { }); it('schedules a recompute when a list is deleted from a segment', async () => { - const precomputed = module.get(PrecomputedSegmentService); + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); service.getSegmentById = jest.fn().mockResolvedValue(newSeg); await service.deleteList(newList.id, newSeg.id, logger); diff --git a/packages/types/src/Experiment/enums.ts b/packages/types/src/Experiment/enums.ts index 3ad8f3ff16..e2206c258c 100644 --- a/packages/types/src/Experiment/enums.ts +++ b/packages/types/src/Experiment/enums.ts @@ -340,7 +340,7 @@ export enum CACHE_PREFIX { GLOBAL_EXCLUDE_SEGMENT_KEY_PREFIX = 'globalExcludeSegment-', MARK_KEY_PREFIX = 'markExperiments-', FEATURE_FLAG_KEY_PREFIX = 'featureFlags-', - PRECOMPUTED_SEGMENT_KEY_PREFIX = 'precomputedSegments-', + FEATURE_FLAG_PRECOMPUTED_SEGMENT_KEY_PREFIX = 'featureFlagPrecomputedSegments-', } export enum STATUS_INDICATOR_CHIP_TYPE { From eeab8705e4da8dbc1020e5688edc77c73daa2576 Mon Sep 17 00:00:00 2001 From: doswalt Date: Wed, 1 Jul 2026 14:22:59 -0400 Subject: [PATCH 07/16] make linter happy --- .../services/FeatureFlagPrecomputedSegmentService.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/backend/test/unit/services/FeatureFlagPrecomputedSegmentService.test.ts b/packages/backend/test/unit/services/FeatureFlagPrecomputedSegmentService.test.ts index 38bc84ec59..b504742ff1 100644 --- a/packages/backend/test/unit/services/FeatureFlagPrecomputedSegmentService.test.ts +++ b/packages/backend/test/unit/services/FeatureFlagPrecomputedSegmentService.test.ts @@ -111,7 +111,9 @@ describe('FeatureFlagPrecomputedSegmentService', () => { expect.objectContaining({ where: { featureFlag: { id: 'flag1' }, enabled: true } }) ); // cache for this flag is invalidated - expect(cacheService.delCache).toHaveBeenCalledWith(CACHE_PREFIX.FEATURE_FLAG_PRECOMPUTED_SEGMENT_KEY_PREFIX + 'flag1'); + expect(cacheService.delCache).toHaveBeenCalledWith( + CACHE_PREFIX.FEATURE_FLAG_PRECOMPUTED_SEGMENT_KEY_PREFIX + 'flag1' + ); }); it('produces empty arrays when the flag has no enabled lists', async () => { From 2f87adbb6dfba749c3e96de7792a6d6fd9797f61 Mon Sep 17 00:00:00 2001 From: doswalt Date: Thu, 2 Jul 2026 12:17:16 -0400 Subject: [PATCH 08/16] perf(flags): lighten flag detail load and list add/edit for large lists Feature flags whose inclusion/exclusion lists have tens of thousands of members made the details page, list edits, and enable/disable toggles slow or broken. This reworks those paths to avoid loading and rewriting full member lists unnecessarily. Detail page load: - Add FeatureFlagService.findOneForDetails (counts-only via loadRelationCountAndMap, no member arrays, no Cartesian join) and use it for GET /flags/:id. findOne keeps loading full members for callers that need them (exports). updateList also uses the counts-only fetch since it only needs the flag id/name. - Frontend renders individualForSegmentCount / groupForSegmentCount; drops the members-dependent values tooltip. Editing a list: - Add GET /segments/:id/members (getSegmentByIdWithMembers) that returns a segment, including private lists, with its members; the edit modal lazy-loads through it. getSegmentById still excludes private segments. - Clear existing members on update with a single DELETE ... WHERE segmentId = :id per member table instead of a 20k-element per-row delete criteria (and drop the pre-SELECT). Enable/disable toggle: - Add PATCH /flags/{inclusion,exclusion}List/:id/status (updateListStatus) that flips only the enabled column without rewriting members; wire the inclusions toggle to it. Add/Edit modal button: - Set the upsert-loading flag on list update actions (not just add/delete), expose the correct feature-flag selector, and combine loading across the flag/experiment/segment stores so the primary button disables during any in-flight add or edit and can't be double-submitted. Tests: unit coverage for findOneForDetails, updateListStatus, getSegmentByIdWithMembers, delete-by-segmentId, and the new controller routes; the FeatureFlag inclusion/exclusion integration case now also exercises the counts-only view, private members fetch, and status toggle. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../api/controllers/FeatureFlagController.ts | 96 ++++++++++++++++- .../src/api/controllers/SegmentController.ts | 37 +++++++ .../validators/FeatureFlagListValidator.ts | 6 ++ packages/backend/src/api/models/Segment.ts | 6 ++ .../src/api/services/FeatureFlagService.ts | 101 +++++++++++++++++- .../src/api/services/SegmentService.ts | 46 ++++---- .../FeatureFlagInclusionExclusion.ts | 40 +++++++ .../controllers/FeatureFlagController.test.ts | 26 +++++ .../controllers/SegmentController.test.ts | 8 ++ .../mocks/FeatureFlagServiceMock.ts | 14 +++ .../controllers/mocks/SegmentServiceMock.ts | 4 + .../unit/services/FeatureFlagService.test.ts | 52 +++++++++ .../test/unit/services/SegmentService.test.ts | 16 +++ .../core/experiments/experiments.service.ts | 2 + .../experiments/store/experiments.model.ts | 1 + .../experiments/store/experiments.reducer.ts | 1 + .../store/experiments.selectors.ts | 5 + .../feature-flags.data.service.ts | 10 ++ .../feature-flags/feature-flags.service.ts | 11 +- .../store/feature-flags.actions.ts | 30 ++++++ .../store/feature-flags.effects.ts | 42 +++++++- .../store/feature-flags.reducer.ts | 52 +++++++++ .../store/feature-flags.selectors.ts | 5 + .../core/segments/segments.data.service.ts | 8 ++ .../src/app/core/segments/segments.service.ts | 7 ++ .../app/core/segments/store/segments.model.ts | 4 + ...-flag-inclusions-section-card.component.ts | 52 ++------- ...rt-private-segment-list-modal.component.ts | 72 ++++++++++--- ...ails-participant-list-table.component.html | 7 +- ...etails-participant-list-table.component.ts | 26 +---- 30 files changed, 674 insertions(+), 113 deletions(-) diff --git a/packages/backend/src/api/controllers/FeatureFlagController.ts b/packages/backend/src/api/controllers/FeatureFlagController.ts index b9851e316b..10c74754d5 100644 --- a/packages/backend/src/api/controllers/FeatureFlagController.ts +++ b/packages/backend/src/api/controllers/FeatureFlagController.ts @@ -30,7 +30,7 @@ import { IdValidator, } from './validators/FeatureFlagValidator'; import { ExperimentUserService } from '../services/ExperimentUserService'; -import { FeatureFlagListValidator } from './validators/FeatureFlagListValidator'; +import { FeatureFlagListValidator, FeatureFlagListStatusValidator } from './validators/FeatureFlagListValidator'; import { Segment } from '../models/Segment'; import { Response } from 'express'; import { UserDTO } from '../DTO/UserDTO'; @@ -214,7 +214,7 @@ export class FeatureFlagsController { @Params({ validate: true }) { id }: IdValidator, @Req() request: AppRequest ): Promise { - return this.featureFlagService.findOne(id, request.logger); + return this.featureFlagService.findOneForDetails(id, request.logger); } /** @@ -630,6 +630,98 @@ export class FeatureFlagsController { return this.featureFlagService.updateList(inclusionList, LIST_FILTER_MODE.INCLUSION, currentUser, request.logger); } + /** + * @swagger + * /flags/inclusionList/{id}/status: + * patch: + * description: Toggle the enabled status of a Feature Flag inclusion list without modifying its members + * consumes: + * - application/json + * parameters: + * - in: path + * name: id + * required: true + * schema: + * type: string + * description: Segment id of the list + * - in: body + * name: status + * description: New enabled status + * schema: + * type: object + * properties: + * enabled: + * type: boolean + * tags: + * - Feature Flags + * produces: + * - application/json + * responses: + * '200': + * description: Feature flag inclusion list status is updated + */ + @Patch('/inclusionList/:id/status') + public async updateInclusionListStatus( + @Params({ validate: true }) { id }: IdValidator, + @Body({ validate: true }) { enabled }: FeatureFlagListStatusValidator, + @CurrentUser() currentUser: UserDTO, + @Req() request: AppRequest + ): Promise { + return this.featureFlagService.updateListStatus( + id, + enabled, + LIST_FILTER_MODE.INCLUSION, + currentUser, + request.logger + ); + } + + /** + * @swagger + * /flags/exclusionList/{id}/status: + * patch: + * description: Toggle the enabled status of a Feature Flag exclusion list without modifying its members + * consumes: + * - application/json + * parameters: + * - in: path + * name: id + * required: true + * schema: + * type: string + * description: Segment id of the list + * - in: body + * name: status + * description: New enabled status + * schema: + * type: object + * properties: + * enabled: + * type: boolean + * tags: + * - Feature Flags + * produces: + * - application/json + * responses: + * '200': + * description: Feature flag exclusion list status is updated + */ + @Patch('/exclusionList/:id/status') + public async updateExclusionListStatus( + @Params({ validate: true }) { id }: IdValidator, + @Body({ validate: true }) { enabled }: FeatureFlagListStatusValidator, + @CurrentUser() currentUser: UserDTO, + @Req() request: AppRequest + ): Promise { + return this.featureFlagService.updateListStatus( + id, + enabled, + LIST_FILTER_MODE.EXCLUSION, + currentUser, + request.logger + ); + } + /** * @swagger * /flags/inclusionList: diff --git a/packages/backend/src/api/controllers/SegmentController.ts b/packages/backend/src/api/controllers/SegmentController.ts index 134147bde2..980ec9488d 100644 --- a/packages/backend/src/api/controllers/SegmentController.ts +++ b/packages/backend/src/api/controllers/SegmentController.ts @@ -412,6 +412,43 @@ export class SegmentController { return segment; } + /** + * @swagger + * /segments/{segmentId}/members: + * get: + * description: Get a segment (including private lists) by id with its full member lists + * tags: + * - Segment + * produces: + * - application/json + * parameters: + * - in: path + * name: segmentId + * description: Segment id + * required: true + * schema: + * type: string + * responses: + * '200': + * description: Get segment with members by id + * schema: + * $ref: '#/definitions/segmentResponse' + * '404': + * description: Segment not found + */ + @Get('/:segmentId/members') + public async getSegmentByIdWithMembers( + @Params({ validate: true }) { segmentId }: IdValidator, + @Req() request: AppRequest + ): Promise { + const segment = await this.segmentService.getSegmentByIdWithMembers(segmentId, request.logger); + if (!segment) { + throw new NotFoundException('Segment not found.'); + } + + return segment; + } + /** * @swagger * /segments/status/{segmentId}: diff --git a/packages/backend/src/api/controllers/validators/FeatureFlagListValidator.ts b/packages/backend/src/api/controllers/validators/FeatureFlagListValidator.ts index 1e9a057fe6..a07afa0d81 100644 --- a/packages/backend/src/api/controllers/validators/FeatureFlagListValidator.ts +++ b/packages/backend/src/api/controllers/validators/FeatureFlagListValidator.ts @@ -18,3 +18,9 @@ export class FeatureFlagListValidator { @Type(() => SegmentInputValidator) public segment: SegmentInputValidator; } + +export class FeatureFlagListStatusValidator { + @IsDefined() + @IsBoolean() + public enabled: boolean; +} diff --git a/packages/backend/src/api/models/Segment.ts b/packages/backend/src/api/models/Segment.ts index 38101daa29..a8131023f9 100644 --- a/packages/backend/src/api/models/Segment.ts +++ b/packages/backend/src/api/models/Segment.ts @@ -48,6 +48,12 @@ export class Segment extends BaseModel { @Type(() => GroupForSegment) public groupForSegment: GroupForSegment[]; + // Not persisted columns. Populated via loadRelationCountAndMap when a segment is loaded + // without its member lists (e.g. the feature-flag details page), so the UI can show counts + // without shipping the full individualForSegment / groupForSegment arrays. + public individualForSegmentCount?: number; + public groupForSegmentCount?: number; + @ManyToMany(() => Segment, (segment) => segment.subSegments) @JoinTable({ name: 'segment_for_segment', diff --git a/packages/backend/src/api/services/FeatureFlagService.ts b/packages/backend/src/api/services/FeatureFlagService.ts index 5a6063d984..299e1be182 100644 --- a/packages/backend/src/api/services/FeatureFlagService.ts +++ b/packages/backend/src/api/services/FeatureFlagService.ts @@ -155,6 +155,35 @@ export class FeatureFlagService { return featureFlag; } + // Lightweight variant of findOne for the details page, which only renders member *counts* + // per inclusion/exclusion list. It skips loading the (potentially tens of thousands of rows) + // individualForSegment and groupForSegment collections — which in findOne also cause a + // Cartesian-product row explosion — and instead maps lightweight COUNT subqueries onto each + // segment (individualForSegmentCount / groupForSegmentCount). The full member lists are + // fetched on demand via GET /segments/:id/members when a list is opened for editing. + // NOTE: callers that need the actual members (e.g. exports) must use findOne, not this. + public async findOneForDetails(id: string, logger?: UpgradeLogger): Promise { + if (logger) { + logger.info({ message: `Find feature flag (details view) by id => ${id}` }); + } + const featureFlag = await this.featureFlagRepository + .createQueryBuilder('feature_flag') + .leftJoinAndSelect('feature_flag.featureFlagSegmentInclusion', 'featureFlagSegmentInclusion') + .leftJoinAndSelect('featureFlagSegmentInclusion.segment', 'segmentInclusion') + .loadRelationCountAndMap('segmentInclusion.individualForSegmentCount', 'segmentInclusion.individualForSegment') + .loadRelationCountAndMap('segmentInclusion.groupForSegmentCount', 'segmentInclusion.groupForSegment') + .leftJoinAndSelect('segmentInclusion.subSegments', 'subSegment') + .leftJoinAndSelect('feature_flag.featureFlagSegmentExclusion', 'featureFlagSegmentExclusion') + .leftJoinAndSelect('featureFlagSegmentExclusion.segment', 'segmentExclusion') + .loadRelationCountAndMap('segmentExclusion.individualForSegmentCount', 'segmentExclusion.individualForSegment') + .loadRelationCountAndMap('segmentExclusion.groupForSegmentCount', 'segmentExclusion.groupForSegment') + .leftJoinAndSelect('segmentExclusion.subSegments', 'subSegmentExclusion') + .where({ id }) + .getOne(); + + return featureFlag; + } + public async create( flagDTO: FeatureFlagValidation, currentUser: UserDTO, @@ -698,9 +727,11 @@ export class FeatureFlagService { logger.info({ message: `Update ${filterType} list for feature flag` }); await this.cacheService.resetPrefixCache(CACHE_PREFIX.FEATURE_FLAG_KEY_PREFIX); return await this.dataSource.transaction(async (transactionalEntityManager) => { - // Find the existing record + // Find the existing record. Only the flag id/name are needed here (for the audit log + // below), so use the counts-only variant to avoid loading every list's members — which + // for large lists made saving an edit very slow. let existingRecord: FeatureFlagSegmentInclusion | FeatureFlagSegmentExclusion; - const featureFlag = await this.findOne(listInput.id); + const featureFlag = await this.findOneForDetails(listInput.id); if (filterType === LIST_FILTER_MODE.INCLUSION) { existingRecord = await this.featureFlagSegmentInclusionRepository.findOne({ @@ -805,6 +836,72 @@ export class FeatureFlagService { }); } + public async updateListStatus( + segmentId: string, + enabled: boolean, + filterType: LIST_FILTER_MODE, + currentUser: UserDTO, + logger: UpgradeLogger + ): Promise { + logger.info({ message: `Update ${filterType} list status for feature flag => segment ${segmentId}` }); + + let existingRecord: FeatureFlagSegmentInclusion | FeatureFlagSegmentExclusion; + if (filterType === LIST_FILTER_MODE.INCLUSION) { + existingRecord = await this.featureFlagSegmentInclusionRepository.findOne({ + where: { segment: { id: segmentId } }, + relations: ['featureFlag', 'segment'], + }); + } else { + existingRecord = await this.featureFlagSegmentExclusionRepository.findOne({ + where: { segment: { id: segmentId } }, + relations: ['featureFlag', 'segment'], + }); + } + + if (!existingRecord) { + const error = new Error(`No existing ${filterType} record found for segment ${segmentId}`); + (error as any).type = SERVER_ERROR.QUERY_FAILED; + logger.error(error); + throw error; + } + + const statusChanged = existingRecord.enabled !== enabled; + existingRecord.enabled = enabled; + + try { + if (filterType === LIST_FILTER_MODE.INCLUSION) { + await this.featureFlagSegmentInclusionRepository.save(existingRecord); + } else { + await this.featureFlagSegmentExclusionRepository.save(existingRecord); + } + } catch (err) { + const error = new Error(`Error in updating ${filterType} list status: ${err}`); + (error as any).type = SERVER_ERROR.QUERY_FAILED; + logger.error(error); + throw error; + } + + await this.clearCachedFlagsForContext(existingRecord.featureFlag.context[0]); + + if (statusChanged) { + const listData: ListOperationsData = { + listId: existingRecord.segment.id, + listName: existingRecord.segment.name, + filterType: filterType, + enabled: enabled, + operation: FEATURE_FLAG_LIST_OPERATION.STATUS_CHANGED, + }; + const updateAuditLog: FeatureFlagUpdatedData = { + flagId: existingRecord.featureFlag.id, + flagName: existingRecord.featureFlag.name, + list: listData, + }; + await this.experimentAuditLogRepository.saveRawJson(LOG_TYPE.FEATURE_FLAG_UPDATED, updateAuditLog, currentUser); + } + + return existingRecord; + } + private paginatedSearchString(params: IFeatureFlagSearchParams): string { const type = params.key; // escape % and ' characters diff --git a/packages/backend/src/api/services/SegmentService.ts b/packages/backend/src/api/services/SegmentService.ts index 51cf74b0c4..40273c9ecb 100644 --- a/packages/backend/src/api/services/SegmentService.ts +++ b/packages/backend/src/api/services/SegmentService.ts @@ -150,6 +150,21 @@ export class SegmentService { return segmentDoc; } + // Fetches a segment (including private lists) by id with its full member lists. Unlike + // getSegmentById this does not exclude private segments, so a feature flag or experiment's + // private inclusion/exclusion list can be loaded on demand for editing (its members are not + // loaded on the counts-only details page). + public async getSegmentByIdWithMembers(id: string, logger: UpgradeLogger): Promise { + logger.info({ message: `Find segment (including private) with members by id. segmentId: ${id}` }); + return this.segmentRepository + .createQueryBuilder('segment') + .leftJoinAndSelect('segment.individualForSegment', 'individualForSegment') + .leftJoinAndSelect('segment.groupForSegment', 'groupForSegment') + .leftJoinAndSelect('segment.subSegments', 'subSegment') + .where({ id }) + .getOne(); + } + public async getSegmentByIds(ids: string[]): Promise { return this.cacheService.wrapFunction(CACHE_PREFIX.SEGMENT_KEY_PREFIX, ids, async () => { const result = await this.segmentRepository @@ -884,31 +899,16 @@ export class SegmentService { ): Promise { let segmentDoc: Segment; - let usersToDelete = [], - groupsToDelete = []; if (segment.id) { try { - // get segment by ids - segmentDoc = await transactionalEntityManager.getRepository(Segment).findOne({ - where: { id: segment.id }, - relations: ['individualForSegment', 'groupForSegment', 'subSegments'], - }); - - // delete individual for segment - if (segmentDoc && segmentDoc.individualForSegment && segmentDoc.individualForSegment.length > 0) { - usersToDelete = segmentDoc.individualForSegment.map((individual) => { - return { userId: individual.userId, segment: segment }; - }); - await transactionalEntityManager.getRepository(IndividualForSegment).delete(usersToDelete as any); - } - - // delete group for segment - if (segmentDoc && segmentDoc.groupForSegment && segmentDoc.groupForSegment.length > 0) { - groupsToDelete = segmentDoc.groupForSegment.map((group) => { - return { groupId: group.groupId, type: group.type, segment: segment }; - }); - await transactionalEntityManager.getRepository(GroupForSegment).delete(groupsToDelete as any); - } + // Clear the existing members before re-inserting them below (the update model is a full + // replace). We delete with a single "WHERE segmentId = :id" statement per member table. + // Previously this fetched every member and passed a per-row criteria array to .delete(), + // which TypeORM expands into one giant OR predicate — extremely slow for large lists. + await Promise.all([ + transactionalEntityManager.getRepository(IndividualForSegment).delete({ segmentId: segment.id }), + transactionalEntityManager.getRepository(GroupForSegment).delete({ segmentId: segment.id }), + ]); } catch (err) { const error = err as ErrorWithType; error.details = 'Error in deleting segment from DB'; diff --git a/packages/backend/test/integration/FeatureFlags/FeatureFlagInclusionExclusion.ts b/packages/backend/test/integration/FeatureFlags/FeatureFlagInclusionExclusion.ts index b025f0bffb..8ac24d52b4 100644 --- a/packages/backend/test/integration/FeatureFlags/FeatureFlagInclusionExclusion.ts +++ b/packages/backend/test/integration/FeatureFlags/FeatureFlagInclusionExclusion.ts @@ -1,6 +1,7 @@ import { Container } from 'typedi'; import { UpgradeLogger } from '../../../src/lib/logger/UpgradeLogger'; import { FeatureFlagService } from '../../../src/api/services/FeatureFlagService'; +import { SegmentService } from '../../../src/api/services/SegmentService'; import { featureFlag } from '../mockData/featureFlag'; import { experimentUsers } from '../mockData/experimentUsers/index'; import { LIST_FILTER_MODE, SEGMENT_TYPE } from 'upgrade_types'; @@ -119,4 +120,43 @@ export default async function FeatureFlagInclusionExclusionLogic(): Promise(SegmentService); + const inclusionSegmentId = detailsInclusionSegment.id; + const segmentWithMembers = await segmentService.getSegmentByIdWithMembers(inclusionSegmentId, new UpgradeLogger()); + expect(segmentWithMembers).toBeTruthy(); + expect(segmentWithMembers.type).toEqual(SEGMENT_TYPE.PRIVATE); + expect(segmentWithMembers.groupForSegment.length).toEqual(1); + // the plain getSegmentById excludes private lists — which is exactly why /members exists + const publicOnlyLookup = await segmentService.getSegmentById(inclusionSegmentId, new UpgradeLogger()); + expect(publicOnlyLookup).toBeFalsy(); + + // --- updateListStatus: toggles enabled without rewriting the segment's members --- + const toggledRecord = await featureFlagService.updateListStatus( + inclusionSegmentId, + false, + LIST_FILTER_MODE.INCLUSION, + user, + new UpgradeLogger() + ); + expect(toggledRecord.enabled).toEqual(false); + const flagAfterToggle = await featureFlagService.findOne(flag.id, new UpgradeLogger()); + const inclusionAfterToggle = flagAfterToggle.featureFlagSegmentInclusion.find( + (inclusion) => inclusion.segment.id === inclusionSegmentId + ); + expect(inclusionAfterToggle.enabled).toEqual(false); + expect(inclusionAfterToggle.segment.groupForSegment.length).toEqual(1); } diff --git a/packages/backend/test/unit/controllers/FeatureFlagController.test.ts b/packages/backend/test/unit/controllers/FeatureFlagController.test.ts index cc15f375ab..c09f639acc 100644 --- a/packages/backend/test/unit/controllers/FeatureFlagController.test.ts +++ b/packages/backend/test/unit/controllers/FeatureFlagController.test.ts @@ -83,6 +83,14 @@ describe('Feature Flag Controller Testing', () => { .expect(200); }); + test('Get request for /api/flags/id', () => { + return request(app) + .get('/api/flags/' + crypto.randomUUID()) + .set('Accept', 'application/json') + .expect('Content-Type', /json/) + .expect(200); + }); + test('Delete request for /api/flags/id', () => { return request(app) .delete('/api/flags/' + crypto.randomUUID()) @@ -91,6 +99,24 @@ describe('Feature Flag Controller Testing', () => { .expect(200); }); + test('Patch request for /api/flags/inclusionList/id/status', () => { + return request(app) + .patch('/api/flags/inclusionList/' + crypto.randomUUID() + '/status') + .send({ enabled: false }) + .set('Accept', 'application/json') + .expect('Content-Type', /json/) + .expect(200); + }); + + test('Patch request for /api/flags/exclusionList/id/status', () => { + return request(app) + .patch('/api/flags/exclusionList/' + crypto.randomUUID() + '/status') + .send({ enabled: true }) + .set('Accept', 'application/json') + .expect('Content-Type', /json/) + .expect(200); + }); + test('Put request for /api/flags/id', () => { return request(app) .put('/api/flags/' + crypto.randomUUID()) diff --git a/packages/backend/test/unit/controllers/SegmentController.test.ts b/packages/backend/test/unit/controllers/SegmentController.test.ts index 7c47c602ca..bd9fb601fe 100644 --- a/packages/backend/test/unit/controllers/SegmentController.test.ts +++ b/packages/backend/test/unit/controllers/SegmentController.test.ts @@ -68,6 +68,14 @@ describe('Segment Controller Testing', () => { .expect(200); }); + test('Get request for /api/segments/:segmentId/members', () => { + return request(app) + .get(`/api/segments/${crypto.randomUUID()}/members`) + .set('Accept', 'application/json') + .expect('Content-Type', /json/) + .expect(200); + }); + test('Get request for /api/segments/status/:segmentId', () => { return request(app) .get(`/api/segments/status/${crypto.randomUUID()}`) diff --git a/packages/backend/test/unit/controllers/mocks/FeatureFlagServiceMock.ts b/packages/backend/test/unit/controllers/mocks/FeatureFlagServiceMock.ts index 91c65265b1..c90f3320ec 100644 --- a/packages/backend/test/unit/controllers/mocks/FeatureFlagServiceMock.ts +++ b/packages/backend/test/unit/controllers/mocks/FeatureFlagServiceMock.ts @@ -59,6 +59,20 @@ export default class FeatureFlagServiceMock { return Promise.resolve([]); } + public findOneForDetails(id: string, logger: UpgradeLogger): Promise> { + return Promise.resolve({}); + } + + public updateListStatus( + segmentId: string, + enabled: boolean, + filterType: string, + currentUser: unknown, + logger: UpgradeLogger + ): Promise> { + return Promise.resolve({}); + } + public validateFeatureFlagContext(flag: FeatureFlag): boolean { return false; } diff --git a/packages/backend/test/unit/controllers/mocks/SegmentServiceMock.ts b/packages/backend/test/unit/controllers/mocks/SegmentServiceMock.ts index cbab6ddd9d..959e29a5da 100644 --- a/packages/backend/test/unit/controllers/mocks/SegmentServiceMock.ts +++ b/packages/backend/test/unit/controllers/mocks/SegmentServiceMock.ts @@ -15,6 +15,10 @@ export default class SegmentServiceMock { return Promise.resolve([]); } + public getSegmentByIdWithMembers(id: string): Promise> { + return Promise.resolve({}); + } + public getSegmentWithStatusById(id: string): Promise<[]> { return Promise.resolve([]); } diff --git a/packages/backend/test/unit/services/FeatureFlagService.test.ts b/packages/backend/test/unit/services/FeatureFlagService.test.ts index a18840cfdf..c9af84b4a9 100644 --- a/packages/backend/test/unit/services/FeatureFlagService.test.ts +++ b/packages/backend/test/unit/services/FeatureFlagService.test.ts @@ -500,6 +500,58 @@ describe('Feature Flag Service Testing', () => { expect(result).toBeTruthy(); }); + it('should find one flag for the details view', async () => { + const result = await service.findOneForDetails(mockFlag1.id, logger); + expect(result).toEqual(mockFlag1); + }); + + describe('updateListStatus', () => { + it('should update an inclusion list enabled status without rewriting its members', async () => { + const inclusionRepo = module.get(getRepositoryToken(FeatureFlagSegmentInclusionRepository)) as any; + const segmentService = module.get(SegmentService); + inclusionRepo.findOne = jest.fn().mockResolvedValue({ + enabled: false, + featureFlag: { id: mockFlag1.id, name: mockFlag1.name, context: ['context1'] }, + segment: { id: 'segment-1', name: 'list' }, + }); + inclusionRepo.save = jest.fn().mockResolvedValue({}); + mockExperimentAuditLogRepository.saveRawJson.mockClear(); + + const result = await service.updateListStatus('segment-1', true, LIST_FILTER_MODE.INCLUSION, mockUser1, logger); + + expect(result.enabled).toBe(true); + expect(inclusionRepo.save).toHaveBeenCalled(); + // a status-only toggle must NOT re-upsert the segment (which would rewrite all members) + expect(segmentService.upsertSegmentInPipeline).not.toHaveBeenCalled(); + // a status change is recorded in the audit log + expect(mockExperimentAuditLogRepository.saveRawJson).toHaveBeenCalled(); + }); + + it('should update an exclusion list enabled status', async () => { + const exclusionRepo = module.get(getRepositoryToken(FeatureFlagSegmentExclusionRepository)) as any; + exclusionRepo.findOne = jest.fn().mockResolvedValue({ + enabled: true, + featureFlag: { id: mockFlag1.id, name: mockFlag1.name, context: ['context1'] }, + segment: { id: 'segment-2', name: 'list' }, + }); + exclusionRepo.save = jest.fn().mockResolvedValue({}); + + const result = await service.updateListStatus('segment-2', false, LIST_FILTER_MODE.EXCLUSION, mockUser1, logger); + + expect(result.enabled).toBe(false); + expect(exclusionRepo.save).toHaveBeenCalled(); + }); + + it('should throw when no existing list record is found', async () => { + const inclusionRepo = module.get(getRepositoryToken(FeatureFlagSegmentInclusionRepository)) as any; + inclusionRepo.findOne = jest.fn().mockResolvedValue(undefined); + + await expect( + service.updateListStatus('missing-segment', true, LIST_FILTER_MODE.INCLUSION, mockUser1, logger) + ).rejects.toThrow(); + }); + }); + it('should import a feature flag from a valid file', async () => { const result = await service.importFeatureFlags( [{ fileName: 'import.json', fileContent: JSON.stringify(mockFlag4) }], diff --git a/packages/backend/test/unit/services/SegmentService.test.ts b/packages/backend/test/unit/services/SegmentService.test.ts index f3042d969e..fff9d12b32 100644 --- a/packages/backend/test/unit/services/SegmentService.test.ts +++ b/packages/backend/test/unit/services/SegmentService.test.ts @@ -355,6 +355,11 @@ describe('Segment Service Testing', () => { expect(segments).toEqual(seg1); }); + it('should get a segment (including private) with members by id', async () => { + const segment = await service.getSegmentByIdWithMembers(seg1.id, logger); + expect(segment).toEqual(seg1); + }); + it('should get segments by ids', async () => { const segments = await service.getSegmentByIds([seg1.id]); expect(segments).toEqual([seg1]); @@ -513,6 +518,17 @@ describe('Segment Service Testing', () => { expect(segments).toEqual(seg1); }); + it('should clear existing members with a single delete-by-segmentId when editing', async () => { + // Editing is a full replace: members are deleted then re-inserted. The delete must be a + // single "WHERE segmentId = :id" per member table rather than a per-row criteria list. + service.checkIsDuplicateSegmentName = jest.fn().mockResolvedValue(false); + repo.delete = jest.fn(); + + await service.upsertSegment(segVal, logger); + + expect(repo.delete).toHaveBeenCalledWith({ segmentId: segVal.id }); + }); + it('should upsert a segment with trimmed whitespace and removed newline or carriage return', async () => { service.checkIsDuplicateSegmentName = jest.fn().mockResolvedValue(false); const segmentWithIdsToCleanUp = new SegmentInputValidator(); diff --git a/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.service.ts b/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.service.ts index 12e15a88fd..26d06218c5 100644 --- a/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.service.ts +++ b/packages/frontend/projects/upgrade/src/app/core/experiments/experiments.service.ts @@ -23,6 +23,7 @@ import { selectAllExperiment, selectHasInitialExperimentsDataLoaded, selectIsLoadingExperiment, + selectIsLoadingUpsertPrivateSegmentList, selectSelectedExperiment, selectExperimentOverviewDetails, selectSearchExperimentParams, @@ -71,6 +72,7 @@ export class ExperimentService { experiments$: Observable = this.store$.pipe(select(selectAllExperiment)); currentUserEmailAddress$ = this.store$.pipe(select(selectCurrentUserEmail)); isLoadingExperiment$ = this.store$.pipe(select(selectIsLoadingExperiment)); + isLoadingUpsertPrivateSegmentList$ = this.store$.pipe(select(selectIsLoadingUpsertPrivateSegmentList)); selectedExperiment$ = this.store$.pipe(select(selectSelectedExperiment)); selectedExperimentOverviewDetails$ = this.store$.pipe(select(selectExperimentOverviewDetails)); searchParams$ = this.store$.pipe(select(selectSearchExperimentParams)); diff --git a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts index 9900006340..4d0abaa0fd 100644 --- a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts +++ b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.model.ts @@ -632,6 +632,7 @@ export interface ExperimentState { isLoadingImportExperiment: boolean; isLoadingRewardsSummary: boolean; rewardsSummaries: Record; + isLoadingUpsertPrivateSegmentList?: boolean; } export interface State extends AppState { diff --git a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.reducer.ts b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.reducer.ts index ee3b08cf76..95fb31e96a 100644 --- a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.reducer.ts +++ b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.reducer.ts @@ -30,6 +30,7 @@ export const initialState: ExperimentState = { isLoadingImportExperiment: false, isLoadingRewardsSummary: false, rewardsSummaries: {}, + isLoadingUpsertPrivateSegmentList: false, }; const reducer = createReducer( diff --git a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.selectors.ts b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.selectors.ts index e1aa1045da..102e8bcf39 100644 --- a/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.selectors.ts +++ b/packages/frontend/projects/upgrade/src/app/core/experiments/store/experiments.selectors.ts @@ -42,6 +42,11 @@ export const selectAllExperiment = createSelector( export const selectIsLoadingExperiment = createSelector(selectExperimentState, (state) => state.isLoadingExperiment); +export const selectIsLoadingUpsertPrivateSegmentList = createSelector( + selectExperimentState, + (state) => state.isLoadingUpsertPrivateSegmentList +); + export const selectHasInitialExperimentsDataLoaded = createSelector( selectExperimentState, (state) => state.hasInitialExperimentsDataLoaded diff --git a/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.ts index ad9ec4753f..fc751c59dd 100644 --- a/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.ts +++ b/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.data.service.ts @@ -125,6 +125,11 @@ export class FeatureFlagsDataService { return this.http.delete(url); } + updateInclusionListStatus(segmentId: string, enabled: boolean) { + const url = `${API_ENDPOINTS.addFlagInclusionList}/${segmentId}/status`; + return this.http.patch(url, { enabled }); + } + addExclusionList(list: AddPrivateSegmentListRequest): Observable { const url = API_ENDPOINTS.addFlagExclusionList; return this.http.post(url, list); @@ -140,6 +145,11 @@ export class FeatureFlagsDataService { return this.http.delete(url); } + updateExclusionListStatus(segmentId: string, enabled: boolean) { + const url = `${API_ENDPOINTS.addFlagExclusionList}/${segmentId}/status`; + return this.http.patch(url, { enabled }); + } + fetchFeatureFlagGraphInfo(params: { flagId: string; range: DATE_RANGE; clientOffset: number }) { const url = API_ENDPOINTS.featureFlagGraphInfo; return this.http.post(url, params); diff --git a/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts index b958eea94e..64d8324d63 100644 --- a/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts +++ b/packages/frontend/projects/upgrade/src/app/core/feature-flags/feature-flags.service.ts @@ -10,6 +10,7 @@ import { selectSearchKey, selectSearchString, selectIsLoadingUpsertFeatureFlag, + selectIsLoadingUpsertPrivateSegmentList, selectIsLoadingUpdateFeatureFlagStatus, selectSelectedFeatureFlag, selectSearchFeatureFlagParams, @@ -61,7 +62,7 @@ export class FeatureFlagsService { isLoadingFeatureFlagDelete$ = this.store$.pipe(select(selectIsLoadingFeatureFlagDelete)); isLoadingImportFeatureFlag$ = this.store$.pipe(select(selectIsLoadingImportFeatureFlag)); isLoadingUpdateFeatureFlagStatus$ = this.store$.pipe(select(selectIsLoadingUpdateFeatureFlagStatus)); - isLoadingUpsertPrivateSegmentList$ = this.store$.pipe(select(selectIsLoadingUpsertFeatureFlag)); + isLoadingUpsertPrivateSegmentList$ = this.store$.pipe(select(selectIsLoadingUpsertPrivateSegmentList)); featureFlags$ = this.store$.pipe(select(selectAllFeatureFlags)); allFeatureFlags$ = this.store$.pipe(select(selectAllFeatureFlagsSortedByDate)); appContexts$ = this.store$.pipe(select(selectAppContexts)); @@ -186,6 +187,10 @@ export class FeatureFlagsService { this.store$.dispatch(FeatureFlagsActions.actionDeleteFeatureFlagInclusionList({ segmentId })); } + updateFeatureFlagInclusionListStatus(segmentId: string, enabled: boolean) { + this.store$.dispatch(FeatureFlagsActions.actionUpdateFeatureFlagInclusionListStatus({ segmentId, enabled })); + } + addFeatureFlagExclusionPrivateSegmentList(list: AddPrivateSegmentListRequest) { this.store$.dispatch(FeatureFlagsActions.actionAddFeatureFlagExclusionList({ list })); } @@ -198,6 +203,10 @@ export class FeatureFlagsService { this.store$.dispatch(FeatureFlagsActions.actionDeleteFeatureFlagExclusionList({ segmentId })); } + updateFeatureFlagExclusionListStatus(segmentId: string, enabled: boolean) { + this.store$.dispatch(FeatureFlagsActions.actionUpdateFeatureFlagExclusionListStatus({ segmentId, enabled })); + } + setGraphRange(range: DATE_RANGE | null, flagId: string, clientOffset: number) { this.store$.dispatch(FeatureFlagsActions.actionSetFeatureFlagGraphRange({ range, flagId, clientOffset })); } diff --git a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.actions.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.actions.ts index 31f07a5081..30d846ac11 100644 --- a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.actions.ts +++ b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.actions.ts @@ -200,6 +200,21 @@ export const actionUpdateFeatureFlagInclusionListFailure = createAction( props<{ error: any }>() ); +export const actionUpdateFeatureFlagInclusionListStatus = createAction( + '[Feature Flags] Update Feature Flag Inclusion List Status', + props<{ segmentId: string; enabled: boolean }>() +); + +export const actionUpdateFeatureFlagInclusionListStatusSuccess = createAction( + '[Feature Flags] Update Feature Flag Inclusion List Status Success', + props<{ segmentId: string; enabled: boolean }>() +); + +export const actionUpdateFeatureFlagInclusionListStatusFailure = createAction( + '[Feature Flags] Update Feature Flag Inclusion List Status Failure', + props<{ error: any }>() +); + export const actionDeleteFeatureFlagInclusionList = createAction( '[Feature Flags] Delete Feature Flag Inclusion List', props<{ segmentId: string }>() @@ -245,6 +260,21 @@ export const actionUpdateFeatureFlagExclusionListFailure = createAction( props<{ error: any }>() ); +export const actionUpdateFeatureFlagExclusionListStatus = createAction( + '[Feature Flags] Update Feature Flag Exclusion List Status', + props<{ segmentId: string; enabled: boolean }>() +); + +export const actionUpdateFeatureFlagExclusionListStatusSuccess = createAction( + '[Feature Flags] Update Feature Flag Exclusion List Status Success', + props<{ segmentId: string; enabled: boolean }>() +); + +export const actionUpdateFeatureFlagExclusionListStatusFailure = createAction( + '[Feature Flags] Update Feature Flag Exclusion List Status Failure', + props<{ error: any }>() +); + export const actionDeleteFeatureFlagExclusionList = createAction( '[Feature Flags] Delete Feature Flag Exclusion List', props<{ segmentId: string }>() diff --git a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts index 4431795cce..41d2304efe 100644 --- a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts +++ b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.effects.ts @@ -2,7 +2,7 @@ import { FeatureFlagsDataService } from '../feature-flags.data.service'; import { Actions, createEffect, ofType } from '@ngrx/effects'; import { Injectable } from '@angular/core'; import * as FeatureFlagsActions from './feature-flags.actions'; -import { catchError, switchMap, map, filter, withLatestFrom, tap, first } from 'rxjs/operators'; +import { catchError, switchMap, mergeMap, map, filter, withLatestFrom, tap, first } from 'rxjs/operators'; import { FeatureFlag, FeatureFlagsPaginationParams, NUMBER_OF_FLAGS } from './feature-flags.model'; import { DATE_RANGE } from '../../experiments/store/experiments.model'; import { Router } from '@angular/router'; @@ -221,6 +221,26 @@ export class FeatureFlagsEffects { ) ); + updateFeatureFlagInclusionListStatus$ = createEffect(() => + this.actions$.pipe( + ofType(FeatureFlagsActions.actionUpdateFeatureFlagInclusionListStatus), + mergeMap(({ segmentId, enabled }) => { + return this.featureFlagsDataService.updateInclusionListStatus(segmentId, enabled).pipe( + map(() => { + this.notificationService.showSuccess( + this.translate.instant('feature-flags.inclusions.update-success.text') + ); + return FeatureFlagsActions.actionUpdateFeatureFlagInclusionListStatusSuccess({ segmentId, enabled }); + }), + catchError((error) => { + this.notificationService.showError(this.translate.instant('feature-flags.inclusions.update-error.text')); + return of(FeatureFlagsActions.actionUpdateFeatureFlagInclusionListStatusFailure({ error })); + }) + ); + }) + ) + ); + deleteFeatureFlagInclusionList$ = createEffect(() => this.actions$.pipe( ofType(FeatureFlagsActions.actionDeleteFeatureFlagInclusionList), @@ -433,6 +453,26 @@ export class FeatureFlagsEffects { ) ); + updateFeatureFlagExclusionListStatus$ = createEffect(() => + this.actions$.pipe( + ofType(FeatureFlagsActions.actionUpdateFeatureFlagExclusionListStatus), + mergeMap(({ segmentId, enabled }) => { + return this.featureFlagsDataService.updateExclusionListStatus(segmentId, enabled).pipe( + map(() => { + this.notificationService.showSuccess( + this.translate.instant('feature-flags.exclusions.update-success.text') + ); + return FeatureFlagsActions.actionUpdateFeatureFlagExclusionListStatusSuccess({ segmentId, enabled }); + }), + catchError((error) => { + this.notificationService.showError(this.translate.instant('feature-flags.exclusions.update-error.text')); + return of(FeatureFlagsActions.actionUpdateFeatureFlagExclusionListStatusFailure({ error })); + }) + ); + }) + ) + ); + setFeatureFlagGraphRange$ = createEffect( () => this.actions$.pipe( diff --git a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.ts index 3715016d93..0794e7ff10 100644 --- a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.ts +++ b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.reducer.ts @@ -182,6 +182,10 @@ const reducer = createReducer( }), // Feature Flag Inclusion List Update Actions + on(FeatureFlagsActions.actionUpdateFeatureFlagInclusionList, (state) => ({ + ...state, + isLoadingUpsertPrivateSegmentList: true, + })), on(FeatureFlagsActions.actionUpdateFeatureFlagInclusionListSuccess, (state, { listResponse }) => { const { featureFlag } = listResponse; @@ -203,6 +207,28 @@ const reducer = createReducer( isLoadingUpsertPrivateSegmentList: false, }; }), + on(FeatureFlagsActions.actionUpdateFeatureFlagInclusionListFailure, (state) => ({ + ...state, + isLoadingUpsertPrivateSegmentList: false, + })), + + // Feature Flag Inclusion List Status Toggle Actions + on(FeatureFlagsActions.actionUpdateFeatureFlagInclusionListStatusSuccess, (state, { segmentId, enabled }) => { + const updatedSelectedFlag = state.selectedFlag + ? { + ...state.selectedFlag, + featureFlagSegmentInclusion: + state.selectedFlag.featureFlagSegmentInclusion?.map((inclusion) => + inclusion.segment.id === segmentId ? { ...inclusion, enabled } : inclusion + ) ?? [], + } + : state.selectedFlag; + + return { + ...state, + selectedFlag: updatedSelectedFlag, + }; + }), // Feature Flag Inclusion List Delete Actions on(FeatureFlagsActions.actionDeleteFeatureFlagInclusionList, (state) => ({ @@ -259,6 +285,10 @@ const reducer = createReducer( }), // Feature Flag Exclusion List Update Actions + on(FeatureFlagsActions.actionUpdateFeatureFlagExclusionList, (state) => ({ + ...state, + isLoadingUpsertPrivateSegmentList: true, + })), on(FeatureFlagsActions.actionUpdateFeatureFlagExclusionListSuccess, (state, { listResponse }) => { const { featureFlag } = listResponse; @@ -280,6 +310,28 @@ const reducer = createReducer( isLoadingUpsertPrivateSegmentList: false, }; }), + on(FeatureFlagsActions.actionUpdateFeatureFlagExclusionListFailure, (state) => ({ + ...state, + isLoadingUpsertPrivateSegmentList: false, + })), + + // Feature Flag Exclusion List Status Toggle Actions + on(FeatureFlagsActions.actionUpdateFeatureFlagExclusionListStatusSuccess, (state, { segmentId, enabled }) => { + const updatedSelectedFlag = state.selectedFlag + ? { + ...state.selectedFlag, + featureFlagSegmentExclusion: + state.selectedFlag.featureFlagSegmentExclusion?.map((exclusion) => + exclusion.segment.id === segmentId ? { ...exclusion, enabled } : exclusion + ) ?? [], + } + : state.selectedFlag; + + return { + ...state, + selectedFlag: updatedSelectedFlag, + }; + }), // Feature Flag Exclusion List Delete Actions on(FeatureFlagsActions.actionDeleteFeatureFlagExclusionList, (state) => ({ diff --git a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.selectors.ts b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.selectors.ts index 52bba20bc8..b413120939 100644 --- a/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.selectors.ts +++ b/packages/frontend/projects/upgrade/src/app/core/feature-flags/store/feature-flags.selectors.ts @@ -48,6 +48,11 @@ export const selectIsLoadingUpsertFeatureFlag = createSelector( (state) => state.isLoadingUpsertFeatureFlag ); +export const selectIsLoadingUpsertPrivateSegmentList = createSelector( + selectFeatureFlagsState, + (state) => state.isLoadingUpsertPrivateSegmentList +); + export const selectDuplicateKeyFound = createSelector(selectFeatureFlagsState, (state) => state.duplicateKeyFound); export const selectSelectedFeatureFlag = createSelector( diff --git a/packages/frontend/projects/upgrade/src/app/core/segments/segments.data.service.ts b/packages/frontend/projects/upgrade/src/app/core/segments/segments.data.service.ts index c0d3a36660..714d3dc449 100644 --- a/packages/frontend/projects/upgrade/src/app/core/segments/segments.data.service.ts +++ b/packages/frontend/projects/upgrade/src/app/core/segments/segments.data.service.ts @@ -48,6 +48,14 @@ export class SegmentsDataService { return this.http.get(url); } + // Fetches a single segment including its full member lists. Used to lazy-load members when + // editing a list, since the details page loads segments with counts only (no member arrays). + // Uses the /members endpoint, which (unlike GET /segments/:id) also returns private lists. + fetchSegmentWithMembersById(id: string): Observable { + const url = `${API_ENDPOINTS.segments}/${id}/members`; + return this.http.get(url); + } + deleteSegment(id: string) { const url = `${API_ENDPOINTS.segments}/${id}`; return this.http.delete(url); diff --git a/packages/frontend/projects/upgrade/src/app/core/segments/segments.service.ts b/packages/frontend/projects/upgrade/src/app/core/segments/segments.service.ts index 8357f49e64..d2e1d98ef8 100644 --- a/packages/frontend/projects/upgrade/src/app/core/segments/segments.service.ts +++ b/packages/frontend/projects/upgrade/src/app/core/segments/segments.service.ts @@ -34,6 +34,7 @@ import { AddSegmentRequest, EditPrivateSegmentListRequest, LIST_OPTION_TYPE, + Segment, SegmentInput, SegmentLocalStorageKeys, UpdateSegmentRequest, @@ -122,6 +123,12 @@ export class SegmentsService { this.store$.dispatch(SegmentsActions.actionGetSegmentById({ segmentId })); } + // Fetches a single segment with its full member lists directly (bypassing the store), used + // to lazy-load members when editing a list loaded from the counts-only details endpoint. + fetchSegmentWithMembersById(segmentId: string): Observable { + return this.segmentsDataService.fetchSegmentWithMembersById(segmentId); + } + refetchCurrentSelectedSegment() { this.selectedSegment$.pipe(take(1)).subscribe((segment) => { if (segment) { diff --git a/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.model.ts b/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.model.ts index aafb3bb37d..82d43a61aa 100644 --- a/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.model.ts +++ b/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.model.ts @@ -107,6 +107,10 @@ export interface Segment { individualForSegment: IndividualForSegment[]; groupForSegment: GroupForSegment[]; subSegments: Segment[]; + // Lightweight member counts returned when the segment is loaded without its full member + // lists (e.g. the feature-flag details page). Undefined when the full lists are present. + individualForSegmentCount?: number; + groupForSegmentCount?: number; listType?: MemberTypes | string; type: SEGMENT_TYPE; status: SEGMENT_STATUS; diff --git a/packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-details-page/feature-flag-details-page-content/feature-flag-inclusions-section-card/feature-flag-inclusions-section-card.component.ts b/packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-details-page/feature-flag-details-page-content/feature-flag-inclusions-section-card/feature-flag-inclusions-section-card.component.ts index 235df40d29..369475e629 100644 --- a/packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-details-page/feature-flag-details-page-content/feature-flag-inclusions-section-card/feature-flag-inclusions-section-card.component.ts +++ b/packages/frontend/projects/upgrade/src/app/features/dashboard/feature-flags/pages/feature-flag-details-page/feature-flag-details-page-content/feature-flag-inclusions-section-card/feature-flag-inclusions-section-card.component.ts @@ -6,7 +6,7 @@ import { } from '@shared-component-lib'; import { TranslateModule } from '@ngx-translate/core'; import { CommonModule } from '@angular/common'; -import { IMenuButtonItem, FILTER_MODE, SEGMENT_TYPE } from 'upgrade_types'; +import { IMenuButtonItem, FILTER_MODE } from 'upgrade_types'; import { FeatureFlagInclusionsTableComponent } from './feature-flag-inclusions-table/feature-flag-inclusions-table.component'; import { FeatureFlagsService } from '../../../../../../../core/feature-flags/feature-flags.service'; import { DialogService } from '../../../../../../../shared/services/common-dialog.service'; @@ -21,11 +21,7 @@ import { ParticipantListRowActionEvent, ParticipantListTableRow, } from '../../../../../../../core/feature-flags/store/feature-flags.model'; -import { - EditPrivateSegmentListDetails, - EditPrivateSegmentListRequest, - Segment, -} from '../../../../../../../core/segments/store/segments.model'; +import { Segment } from '../../../../../../../core/segments/store/segments.model'; import { UserPermission } from '../../../../../../../core/auth/store/auth.models'; import { AuthService } from '../../../../../../../core/auth/auth.service'; @@ -164,10 +160,10 @@ export class FeatureFlagInclusionsSectionCardComponent { onRowAction(event: ParticipantListRowActionEvent, flagId: string): void { switch (event.action) { case PARTICIPANT_LIST_ROW_ACTION.ENABLE: - this.onEnableIncludeList(event.rowData, flagId); + this.onEnableIncludeList(event.rowData); break; case PARTICIPANT_LIST_ROW_ACTION.DISABLE: - this.onDisableIncludeList(event.rowData, flagId); + this.onDisableIncludeList(event.rowData); break; case PARTICIPANT_LIST_ROW_ACTION.EDIT: this.onEditIncludeList(event.rowData, flagId); @@ -178,24 +174,24 @@ export class FeatureFlagInclusionsSectionCardComponent { } } - onEnableIncludeList(rowData: ParticipantListTableRow, flagId: string): void { + onEnableIncludeList(rowData: ParticipantListTableRow): void { this.dialogService .openEnableIncludeListModal(rowData.segment.name) .afterClosed() .subscribe((confirmClicked) => { if (confirmClicked) { - this.sendUpdateIncludeListRequest(flagId, true, rowData.listType, rowData.segment); + this.featureFlagService.updateFeatureFlagInclusionListStatus(rowData.segment.id, true); } }); } - onDisableIncludeList(rowData: ParticipantListTableRow, flagId: string): void { + onDisableIncludeList(rowData: ParticipantListTableRow): void { this.dialogService .openDisableIncludeListModal(rowData.segment.name) .afterClosed() .subscribe((confirmClicked) => { if (confirmClicked) { - this.sendUpdateIncludeListRequest(flagId, false, rowData.listType, rowData.segment); + this.featureFlagService.updateFeatureFlagInclusionListStatus(rowData.segment.id, false); } }); } @@ -204,38 +200,6 @@ export class FeatureFlagInclusionsSectionCardComponent { this.dialogService.openFeatureFlagEditIncludeListModal(rowData, rowData.segment.context, flagId); } - sendUpdateIncludeListRequest(flagId: string, enabled: boolean, listType: string, segment: Segment): void { - const list: EditPrivateSegmentListDetails = this.createEditPrivateSegmentListDetails(segment); - - const listRequest: EditPrivateSegmentListRequest = { - id: flagId, - enabled, - listType, - segment: list, - }; - - this.sendUpdateFeatureFlagInclusionRequest(listRequest); - } - - createEditPrivateSegmentListDetails(segment: Segment): EditPrivateSegmentListDetails { - const editPrivateSegmentListDetails: EditPrivateSegmentListDetails = { - id: segment.id, - name: segment.name, - description: segment.description, - context: segment.context, - type: SEGMENT_TYPE.PRIVATE, - userIds: segment.individualForSegment.map((individual) => individual.userId), - groups: segment.groupForSegment, - subSegmentIds: segment.subSegments.map((subSegment) => subSegment.id), - }; - - return editPrivateSegmentListDetails; - } - - sendUpdateFeatureFlagInclusionRequest(request: EditPrivateSegmentListRequest): void { - this.featureFlagService.updateFeatureFlagInclusionPrivateSegmentList(request); - } - onDeleteIncludeList(segment: Segment): void { this.dialogService .openDeleteIncludeListModal(segment.name) diff --git a/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-private-segment-list-modal/upsert-private-segment-list-modal.component.ts b/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-private-segment-list-modal/upsert-private-segment-list-modal.component.ts index 74ada3f6c3..9427ccfa5f 100644 --- a/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-private-segment-list-modal/upsert-private-segment-list-modal.component.ts +++ b/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-private-segment-list-modal/upsert-private-segment-list-modal.component.ts @@ -1,4 +1,4 @@ -import { ChangeDetectionStrategy, Component, Inject, ViewChild } from '@angular/core'; +import { ChangeDetectionStrategy, ChangeDetectorRef, Component, Inject, ViewChild } from '@angular/core'; import { CommonModalComponent, CommonTagsInputComponent } from '@shared-component-lib'; import { MAT_DIALOG_DATA, MatDialog, MatDialogRef } from '@angular/material/dialog'; import { CommonModule } from '@angular/common'; @@ -35,7 +35,16 @@ import { UpsertPrivateSegmentListParams, } from '../../../../../core/segments/store/segments.model'; import { MatAutocompleteModule } from '@angular/material/autocomplete'; -import { BehaviorSubject, combineLatestWith, map, Observable, startWith, Subscription, timer } from 'rxjs'; +import { + BehaviorSubject, + combineLatest, + combineLatestWith, + map, + Observable, + startWith, + Subscription, + timer, +} from 'rxjs'; import { SEGMENT_TYPE } from '../../../../../../../../../../types/src'; import isEqual from 'lodash.isequal'; import { FeatureFlagsService } from '../../../../../core/feature-flags/feature-flags.service'; @@ -63,7 +72,15 @@ import { SharedModule } from '../../../../../shared/shared.module'; export class UpsertPrivateSegmentListModalComponent { @ViewChild('typeSelectRef') typeSelectRef: MatSelect; listOptionTypes$: Observable<{ value: string; viewValue: string }[]>; - isLoadingUpsertFeatureFlagList$ = this.featureFlagService.isLoadingUpsertPrivateSegmentList$; + // This modal drives add/edit for feature-flag lists, experiment lists, and standalone + // segment lists — each backed by a different store. Disable the primary button while an + // upsert is in flight in ANY of them so it can't be double-submitted. Each store resets its + // own flag on success/failure, so this re-enables (or the modal closes) automatically. + isUpsertLoading$ = combineLatest([ + this.featureFlagService.isLoadingUpsertPrivateSegmentList$, + this.experimentService.isLoadingUpsertPrivateSegmentList$, + this.segmentsService.isLoadingSegments$, + ]).pipe(map((loadingFlags) => loadingFlags.some(Boolean))); initialFormValues$ = new BehaviorSubject(null); subscriptions = new Subscription(); @@ -86,6 +103,7 @@ export class UpsertPrivateSegmentListModalComponent { private experimentService: ExperimentService, private featureFlagService: FeatureFlagsService, private commonExportHelpersService: CommonExportHelpersService, + private changeDetectorRef: ChangeDetectorRef, public dialogRef: MatDialogRef ) {} @@ -187,13 +205,31 @@ export class UpsertPrivateSegmentListModalComponent { return; } - const values = this.determineValues(sourceList.listType, sourceList.segment); + this.applyEditFormValues(sourceList.listType, sourceList.segment); + + // The feature-flag details page loads segments with member counts only (no member + // arrays) to stay lightweight, so lazy-load the full segment when we detect that members + // exist but weren't loaded. Editing then operates on the complete list. + if (this.segmentMembersNeedFetch(sourceList.listType, sourceList.segment)) { + this.subscriptions.add( + this.segmentsService.fetchSegmentWithMembersById(sourceList.segment.id).subscribe((segment) => { + if (segment) { + this.applyEditFormValues(sourceList.listType, segment); + this.changeDetectorRef.markForCheck(); + } + }) + ); + } + } + + private applyEditFormValues(listType: string, segment: Segment): void { + const values = this.determineValues(listType, segment); const formValue: PrivateSegmentListFormData = { - listType: sourceList.listType as LIST_OPTION_TYPE, - segment: sourceList.segment, + listType: listType as LIST_OPTION_TYPE, + segment, values, - name: sourceList.segment.name, - description: sourceList.segment.description, + name: segment.name, + description: segment.description, }; this.privateSegmentListForm.patchValue(formValue, { emitEvent: false }); @@ -202,17 +238,29 @@ export class UpsertPrivateSegmentListModalComponent { this.initialFormValues$.next(formValue); // Trigger validators after populating the form - this.setValidatorsBasedOnListType(sourceList.listType); + this.setValidatorsBasedOnListType(listType); + } + + // True when the segment's member list wasn't loaded (counts-only) but a non-zero count + // indicates members exist, so the full list must be fetched before editing. + private segmentMembersNeedFetch(listType: string, segment: Segment): boolean { + if (!segment?.id || listType === LIST_OPTION_TYPE.SEGMENT) { + return false; + } + if (listType === LIST_OPTION_TYPE.INDIVIDUAL) { + return !segment.individualForSegment?.length && (segment.individualForSegmentCount ?? 0) > 0; + } + return !segment.groupForSegment?.length && (segment.groupForSegmentCount ?? 0) > 0; } determineValues(listType: string, segment: Segment): string[] { switch (listType) { case LIST_OPTION_TYPE.INDIVIDUAL: - return segment.individualForSegment.map((individual) => individual.userId); + return segment.individualForSegment?.map((individual) => individual.userId) ?? []; case LIST_OPTION_TYPE.SEGMENT: return []; default: - return segment.groupForSegment.map((group) => group.groupId); + return segment.groupForSegment?.map((group) => group.groupId) ?? []; } } @@ -231,7 +279,7 @@ export class UpsertPrivateSegmentListModalComponent { } listenForPrimaryButtonDisabled() { - this.isPrimaryButtonDisabled$ = this.isLoadingUpsertFeatureFlagList$.pipe( + this.isPrimaryButtonDisabled$ = this.isUpsertLoading$.pipe( combineLatestWith(this.isInitialFormValueChanged$), map(([isLoading, isInitialFormValueChanged]) => isLoading || !isInitialFormValueChanged) ); diff --git a/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-details-participant-list-table/common-details-participant-list-table.component.html b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-details-participant-list-table/common-details-participant-list-table.component.html index f9671645da..4a0ee1114e 100644 --- a/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-details-participant-list-table/common-details-participant-list-table.component.html +++ b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-details-participant-list-table/common-details-participant-list-table.component.html @@ -27,12 +27,7 @@ @if (rowData?.segment && rowData.listType?.toLowerCase() !== memberTypes.SEGMENT.toLowerCase()) { - + {{ getValuesText(rowData) }} } diff --git a/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-details-participant-list-table/common-details-participant-list-table.component.ts b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-details-participant-list-table/common-details-participant-list-table.component.ts index 1bd7e2e056..f058d92e45 100644 --- a/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-details-participant-list-table/common-details-participant-list-table.component.ts +++ b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-details-participant-list-table/common-details-participant-list-table.component.ts @@ -81,8 +81,6 @@ export class CommonDetailsParticipantListTableComponent { ACTIONS: 'segments.global-actions.text', }; - private readonly MAX_TOOLTIP_VALUES = 10; - ngOnInit() { this.displayedColumns = this.tableType === LIST_FILTER_MODE.INCLUSION @@ -94,10 +92,12 @@ export class CommonDetailsParticipantListTableComponent { const listType = rowData.listType; let count: number; + // Prefer the lightweight count returned by the details endpoint (member lists are not + // loaded there); fall back to the array length when the full lists are present. if (listType?.toLowerCase() === this.memberTypes.INDIVIDUAL.toLowerCase()) { - count = rowData.segment.individualForSegment?.length || 0; + count = rowData.segment.individualForSegmentCount ?? rowData.segment.individualForSegment?.length ?? 0; } else { - count = rowData.segment.groupForSegment?.length || 0; + count = rowData.segment.groupForSegmentCount ?? rowData.segment.groupForSegment?.length ?? 0; } if (count === 0) { @@ -109,24 +109,6 @@ export class CommonDetailsParticipantListTableComponent { } } - getValuesTooltipText(rowData: ParticipantListTableRow): string { - const listType = rowData.listType; - let values: string[]; - - if (listType?.toLowerCase() === this.memberTypes.INDIVIDUAL.toLowerCase()) { - values = rowData.segment.individualForSegment?.map((item) => item.userId) || []; - } else { - values = rowData.segment.groupForSegment?.map((item) => item.groupId) || []; - } - - // Show only first 10 values if there are more - if (values.length > this.MAX_TOOLTIP_VALUES) { - return values.slice(0, this.MAX_TOOLTIP_VALUES).join(', ') + '...'; - } - - return values.join(', '); - } - getFormattedListType(rowData: ParticipantListTableRow): string { const listType = rowData.listType; From 8193311f7cfc57f7a46edf261a8015fee8e0169f Mon Sep 17 00:00:00 2001 From: doswalt Date: Thu, 2 Jul 2026 15:25:46 -0400 Subject: [PATCH 09/16] namespace precomputed segment group IDs with group type Group IDs in feature_flag_precomputed_segment were stored bare in the same flat arrays as individual user IDs, so a group ID could collide with a user ID and flip an include/exclude decision. Matching also ignored group type, diverging from the type-aware experiment / on-the-fly resolution path. Namespace group IDs as `type:groupId` (individuals stay bare) via a shared precomputedGroupKey helper used by both the write and read paths. Add a migration that clears the table so the startup backfill rebuilds every row in the new format; missing rows fall back to on-the-fly resolution in the interim. Co-Authored-By: Claude Opus 4.8 --- .../FeatureFlagPrecomputedSegmentService.ts | 15 ++++++- .../src/api/services/FeatureFlagService.ts | 16 +++++--- ...0000-clearFeatureFlagPrecomputedSegment.ts | 20 +++++++++ ...atureFlagPrecomputedSegmentService.test.ts | 7 ++-- .../unit/services/FeatureFlagService.test.ts | 41 ++++++++++++++++++- 5 files changed, 89 insertions(+), 10 deletions(-) create mode 100644 packages/backend/src/database/migrations/1782950400000-clearFeatureFlagPrecomputedSegment.ts diff --git a/packages/backend/src/api/services/FeatureFlagPrecomputedSegmentService.ts b/packages/backend/src/api/services/FeatureFlagPrecomputedSegmentService.ts index 23d0b30cc7..635cacb0aa 100644 --- a/packages/backend/src/api/services/FeatureFlagPrecomputedSegmentService.ts +++ b/packages/backend/src/api/services/FeatureFlagPrecomputedSegmentService.ts @@ -11,6 +11,18 @@ import { CACHE_PREFIX } from 'upgrade_types'; import { UpgradeLogger } from '../../lib/logger/UpgradeLogger'; import { EntityManager } from 'typeorm'; +// Group IDs are stored namespaced with their group type in the same flat arrays as bare individual +// user IDs. This (a) prevents a group ID from ever colliding with an individual user ID that happens +// to share the same string, and (b) keeps matching type-aware, matching the experiment / on-the-fly +// resolution path (see ExperimentAssignmentService.inclusionExclusionLogic). Both the write path +// here and the read path in FeatureFlagService MUST compose the key with this same helper. The type +// is recoverable by splitting on the FIRST delimiter — group types (e.g. 'schoolId') never contain +// ':', though group IDs may. +export const PRECOMPUTED_GROUP_DELIMITER = ':'; +export function precomputedGroupKey(type: string, groupId: string): string { + return `${type}${PRECOMPUTED_GROUP_DELIMITER}${groupId}`; +} + @Service() export class FeatureFlagPrecomputedSegmentService { constructor( @@ -148,8 +160,9 @@ export class FeatureFlagPrecomputedSegmentService { const subSegmentIds: string[] = []; for (const segment of segments) { + // Individuals are stored bare; groups are namespaced with their type (see precomputedGroupKey). segment.individualForSegment.forEach((ind) => ids.push(ind.userId)); - segment.groupForSegment.forEach((grp) => ids.push(grp.groupId)); + segment.groupForSegment.forEach((grp) => ids.push(precomputedGroupKey(grp.type, grp.groupId))); segment.subSegments.forEach((sub) => { if (!seen.has(sub.id)) subSegmentIds.push(sub.id); }); diff --git a/packages/backend/src/api/services/FeatureFlagService.ts b/packages/backend/src/api/services/FeatureFlagService.ts index 3af1293c61..b896e38df9 100644 --- a/packages/backend/src/api/services/FeatureFlagService.ts +++ b/packages/backend/src/api/services/FeatureFlagService.ts @@ -54,7 +54,7 @@ import { SegmentRepository } from '../repositories/SegmentRepository'; import { ExperimentAuditLog } from '../models/ExperimentAuditLog'; import { NotFoundException } from '@nestjs/common/exceptions'; import { CacheService } from './CacheService'; -import { FeatureFlagPrecomputedSegmentService } from './FeatureFlagPrecomputedSegmentService'; +import { FeatureFlagPrecomputedSegmentService, precomputedGroupKey } from './FeatureFlagPrecomputedSegmentService'; import { SegmentFile, SegmentInputValidator } from '../controllers/validators/SegmentInputValidator'; import dayjs from 'dayjs'; import { getDateRangeNames } from '../repositories/utils/dateQuery'; @@ -920,8 +920,14 @@ export class FeatureFlagService { const flagIds = featureFlags.map((f) => f.id); const precomputedMap = await this.featureFlagPrecomputedSegmentService.getPrecomputedSets(flagIds); - // Flatten all group IDs from the user's group map (type is ignored per design decision) - const userGroupIds: string[] = experimentUser.group ? Object.values(experimentUser.group).flat() : []; + // Build type-qualified group keys from the user's group map so they match the namespaced group + // IDs stored in the precomputed arrays (individuals are matched bare against experimentUser.id). + // Must use the same precomputedGroupKey helper as the write path. + const userGroupKeys: string[] = experimentUser.group + ? Object.entries(experimentUser.group).flatMap(([type, groupIds]) => + groupIds.map((groupId) => precomputedGroupKey(type, groupId)) + ) + : []; // Any flag without a precomputed row falls back to on-the-fly segment resolution so a // truly-missing row never silently produces a wrong include/exclude decision. Seeding on @@ -949,8 +955,8 @@ export class FeatureFlagService { // Individual inclusion bypasses group checks if (inclusionSet.has(experimentUser.id)) return true; - const inGroupExclusion = userGroupIds.some((gid) => exclusionSet.has(gid)); - const inGroupInclusion = userGroupIds.some((gid) => inclusionSet.has(gid)); + const inGroupExclusion = userGroupKeys.some((key) => exclusionSet.has(key)); + const inGroupInclusion = userGroupKeys.some((key) => inclusionSet.has(key)); if (flag.filterMode === FILTER_MODE.INCLUDE_ALL) { return !inGroupExclusion; diff --git a/packages/backend/src/database/migrations/1782950400000-clearFeatureFlagPrecomputedSegment.ts b/packages/backend/src/database/migrations/1782950400000-clearFeatureFlagPrecomputedSegment.ts new file mode 100644 index 0000000000..963d397b3d --- /dev/null +++ b/packages/backend/src/database/migrations/1782950400000-clearFeatureFlagPrecomputedSegment.ts @@ -0,0 +1,20 @@ +import { MigrationInterface, QueryRunner } from 'typeorm'; + +// The stored format of group IDs in feature_flag_precomputed_segment changed: group IDs are now +// namespaced with their group type (see precomputedGroupKey) instead of stored bare. Existing rows +// hold the old bare format and would silently stop matching, so clear the table. The startup +// backfill (backfillMissingFlags) then rebuilds every row in the new format. Until backfill +// completes, flags with a missing row fall back to on-the-fly resolution, so no wrong decisions are +// made in the interim. +export class ClearFeatureFlagPrecomputedSegment1782950400000 implements MigrationInterface { + name = 'ClearFeatureFlagPrecomputedSegment1782950400000'; + + public async up(queryRunner: QueryRunner): Promise { + await queryRunner.query(`DELETE FROM "feature_flag_precomputed_segment"`); + } + + public async down(queryRunner: QueryRunner): Promise { + // Reverting the code reverts the stored format too; clear again so the (old-format) backfill rebuilds. + await queryRunner.query(`DELETE FROM "feature_flag_precomputed_segment"`); + } +} diff --git a/packages/backend/test/unit/services/FeatureFlagPrecomputedSegmentService.test.ts b/packages/backend/test/unit/services/FeatureFlagPrecomputedSegmentService.test.ts index b504742ff1..8c406790f1 100644 --- a/packages/backend/test/unit/services/FeatureFlagPrecomputedSegmentService.test.ts +++ b/packages/backend/test/unit/services/FeatureFlagPrecomputedSegmentService.test.ts @@ -70,7 +70,7 @@ describe('FeatureFlagPrecomputedSegmentService', () => { segA: { id: 'segA', individualForSegment: [{ userId: 'u1' }], - groupForSegment: [{ groupId: 'g1' }], + groupForSegment: [{ groupId: 'g1', type: 'schoolId' }], subSegments: [{ id: 'segChild' }], }, segChild: { @@ -103,8 +103,9 @@ describe('FeatureFlagPrecomputedSegmentService', () => { expect(precomputedSegmentRepository.upsertByFlagId).toHaveBeenCalledTimes(1); const [flagId, inclusionIds, exclusionIds] = precomputedSegmentRepository.upsertByFlagId.mock.calls[0]; expect(flagId).toEqual('flag1'); - // recursive sub-segment member u2 must be included alongside the direct members - expect(inclusionIds.sort()).toEqual(['g1', 'u1', 'u2']); + // recursive sub-segment member u2 must be included alongside the direct members; + // group members are namespaced with their type (schoolId:g1), individuals stay bare + expect(inclusionIds.sort()).toEqual(['schoolId:g1', 'u1', 'u2']); expect(exclusionIds).toEqual(['u3']); // only enabled lists are queried expect(featureFlagSegmentInclusionRepository.find).toHaveBeenCalledWith( diff --git a/packages/backend/test/unit/services/FeatureFlagService.test.ts b/packages/backend/test/unit/services/FeatureFlagService.test.ts index 67aff6af2b..887928502f 100644 --- a/packages/backend/test/unit/services/FeatureFlagService.test.ts +++ b/packages/backend/test/unit/services/FeatureFlagService.test.ts @@ -725,8 +725,9 @@ describe('Feature Flag Service Testing', () => { const precomputed = module.get(FeatureFlagPrecomputedSegmentService); service.cacheService.wrap = jest.fn().mockResolvedValue([fastFlag]); + // stored group IDs are namespaced with their type (classId:bad-class); individuals stay bare (precomputed.getPrecomputedSets as jest.Mock).mockResolvedValue( - new Map([[fastFlag.id, { inclusionIds: ['user123'], exclusionIds: ['bad-class'] }]]) + new Map([[fastFlag.id, { inclusionIds: ['user123'], exclusionIds: ['classId:bad-class'] }]]) ); const result = await service.getKeys(userDoc, 'context1', logger); @@ -734,6 +735,44 @@ describe('Feature Flag Service Testing', () => { expect(result).toEqual([fastFlag.key]); }); + it('matches a group exclusion only when the group type also matches (type-aware)', async () => { + const excludeFlag = { id: 'ex-flag-id', key: 'ex-key', filterMode: FILTER_MODE.INCLUDE_ALL }; + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); + service.cacheService.wrap = jest.fn().mockResolvedValue([excludeFlag]); + + // User is in group 'grpA' under type 'classId'. The stored exclusion targets 'grpA' under a + // DIFFERENT type ('schoolId'), so with type-aware matching the user is NOT excluded. + const wrongType = { id: 'user123', group: { classId: ['grpA'] }, workingGroup: {} } as any; + (precomputed.getPrecomputedSets as jest.Mock).mockResolvedValue( + new Map([[excludeFlag.id, { inclusionIds: [], exclusionIds: ['schoolId:grpA'] }]]) + ); + expect(await service.getKeys(wrongType, 'context1', logger)).toEqual([excludeFlag.key]); + + // Same group ID under the MATCHING type -> excluded. + const rightType = { id: 'user123', group: { schoolId: ['grpA'] }, workingGroup: {} } as any; + (precomputed.getPrecomputedSets as jest.Mock).mockResolvedValue( + new Map([[excludeFlag.id, { inclusionIds: [], exclusionIds: ['schoolId:grpA'] }]]) + ); + expect(await service.getKeys(rightType, 'context1', logger)).toEqual([]); + }); + + it('does not treat a group ID that collides with the user ID as an individual match', async () => { + // A group named the same string as the user's individual ID is excluded. Because groups are + // namespaced (schoolId:user123) and the individual check is bare (user123), the user must NOT + // be individually excluded — they are only excluded if they actually belong to that group. + const collideFlag = { id: 'col-flag-id', key: 'col-key', filterMode: FILTER_MODE.INCLUDE_ALL }; + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); + service.cacheService.wrap = jest.fn().mockResolvedValue([collideFlag]); + + const userDoc = { id: 'user123', group: {}, workingGroup: {} } as any; + (precomputed.getPrecomputedSets as jest.Mock).mockResolvedValue( + new Map([[collideFlag.id, { inclusionIds: [], exclusionIds: ['schoolId:user123'] }]]) + ); + + // Not in the excluded group -> stays included (INCLUDE_ALL) + expect(await service.getKeys(userDoc, 'context1', logger)).toEqual([collideFlag.key]); + }); + it('falls back to on-the-fly resolution when the precomputed row is missing', async () => { const userDoc = { id: 'user123', group: {}, workingGroup: {} } as any; const experimentAssignmentService = module.get(ExperimentAssignmentService); From b4a56f9d334f2b079287b14db7ee38c836165805 Mon Sep 17 00:00:00 2001 From: danoswaltCL <97542869+danoswaltCL@users.noreply.github.com> Date: Thu, 2 Jul 2026 15:28:04 -0400 Subject: [PATCH 10/16] better error log Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- packages/backend/src/api/services/SegmentService.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/backend/src/api/services/SegmentService.ts b/packages/backend/src/api/services/SegmentService.ts index caa3560fad..9c0184d7be 100644 --- a/packages/backend/src/api/services/SegmentService.ts +++ b/packages/backend/src/api/services/SegmentService.ts @@ -492,9 +492,7 @@ export class SegmentService { affectedFlagIds.forEach((flagId) => this.featureFlagPrecomputedSegmentService .recomputeForFlag(flagId, logger) - .catch((err) => - logger.error({ message: `Error recomputing feature_flag_precomputed_segment for flag ${flagId}: ${err}` }) - ) + .catch((err) => logger.error({ message: `Error recomputing feature_flag_precomputed_segment for flag ${flagId}`, error: err })) ); // reset cache From aded2f0d388a3f48e1b4a5ce321e560049a8de12 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 2 Jul 2026 19:29:44 +0000 Subject: [PATCH 11/16] fix: update CLAUDE.md to use correct FeatureFlagPrecomputedSegment names --- packages/backend/CLAUDE.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/backend/CLAUDE.md b/packages/backend/CLAUDE.md index 1ce4231dab..78d95af35b 100644 --- a/packages/backend/CLAUDE.md +++ b/packages/backend/CLAUDE.md @@ -77,16 +77,16 @@ Available at `/swagger` when `SWAGGER_ENABLED=true`. Auto-generated from JSDoc ` ## Precomputed Segment Lists (Feature Flags) -Segment inclusion/exclusion for **feature flags** is precomputed and stored flat in the `precomputed_segment` table rather than resolved on-the-fly at assignment time. +Segment inclusion/exclusion for **feature flags** is precomputed and stored flat in the `feature_flag_precomputed_segment` table rather than resolved on-the-fly at assignment time. ### How it works -- **`PrecomputedSegment` entity** (`src/api/models/PrecomputedSegment.ts`) — one row per feature flag, columns: `featureFlagId` (PK), `inclusionIds: text[]`, `exclusionIds: text[]`. FK to `feature_flag` with `onDelete: CASCADE`. -- **`PrecomputedSegmentService`** (`src/api/services/PrecomputedSegmentService.ts`) — owns all computation and cache logic: +- **`FeatureFlagPrecomputedSegment` entity** (`src/api/models/FeatureFlagPrecomputedSegment.ts`) — one row per feature flag, columns: `featureFlagId` (PK), `inclusionIds: text[]`, `exclusionIds: text[]`. FK to `feature_flag` with `onDelete: CASCADE`. +- **`FeatureFlagPrecomputedSegmentService`** (`src/api/services/FeatureFlagPrecomputedSegmentService.ts`) — owns all computation and cache logic: - `recomputeForFlag(flagId)` — flattens all enabled inclusion/exclusion segments (recursive sub-segments) into flat ID arrays and upserts the row. - `scheduleRecomputeForSegment(segmentId)` — fire-and-forget; finds all flags referencing a segment (and its parents) and calls `recomputeForFlag` for each. - `getAffectedFlagIds(segmentId)` — public helper that returns flag IDs affected by a given segment (used before deletion). - - `getPrecomputedSets(flagIds[])` — cache-wrapped batch fetch, returns a `Map`. + - `getPrecomputedSets(flagIds[])` — cache-wrapped batch fetch, returns a `Map`. - `backfillMissingFlags(logger)` — called at startup; computes rows only for flags that have none yet (no-op once all flags are populated). - `recomputeAllFlags(logger)` — full refresh of every flag; not called automatically, available for manual recovery. - **Assignment read path** — `FeatureFlagService.featureFlagLevelInclusionExclusion()` calls `getPrecomputedSets()` and does in-memory `Set.has()` checks. No recursive segment queries at assignment time. @@ -109,4 +109,4 @@ All recomputes triggered from write paths are **fire-and-forget** — callers ne ### Key invariant -The `precomputed_segment` row must always be recomputed **after** the structural change completes, so the flat arrays reflect the new state. For deletions specifically, affected flag IDs must be collected **before** the delete because the join table records are gone afterward. +The `feature_flag_precomputed_segment` row must always be recomputed **after** the structural change completes, so the flat arrays reflect the new state. For deletions specifically, affected flag IDs must be collected **before** the delete because the join table records are gone afterward. From 26f23d1217fa9d29e201d9d6fbeff643449bffa6 Mon Sep 17 00:00:00 2001 From: doswalt Date: Thu, 2 Jul 2026 20:26:02 -0400 Subject: [PATCH 12/16] make linter happy --- packages/backend/src/api/services/SegmentService.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/backend/src/api/services/SegmentService.ts b/packages/backend/src/api/services/SegmentService.ts index 9c0184d7be..90e0aaa937 100644 --- a/packages/backend/src/api/services/SegmentService.ts +++ b/packages/backend/src/api/services/SegmentService.ts @@ -492,7 +492,9 @@ export class SegmentService { affectedFlagIds.forEach((flagId) => this.featureFlagPrecomputedSegmentService .recomputeForFlag(flagId, logger) - .catch((err) => logger.error({ message: `Error recomputing feature_flag_precomputed_segment for flag ${flagId}`, error: err })) + .catch((err) => + logger.error({ message: `Error recomputing feature_flag_precomputed_segment for flag ${flagId}`, error: err }) + ) ); // reset cache From 9392066458d27aad68380185622cc50c098a43d0 Mon Sep 17 00:00:00 2001 From: doswalt Date: Mon, 6 Jul 2026 11:03:02 -0400 Subject: [PATCH 13/16] guarantee update first and then fire-forget recalculate --- packages/backend/CLAUDE.md | 21 ++++--- .../FeatureFlagPrecomputedSegmentService.ts | 36 +++++++++++ .../src/api/services/FeatureFlagService.ts | 59 ++++++++++--------- .../src/api/services/SegmentService.ts | 24 ++++---- ...0000-clearFeatureFlagPrecomputedSegment.ts | 20 ------- ...atureFlagPrecomputedSegmentService.test.ts | 59 +++++++++++++++++++ .../unit/services/FeatureFlagService.test.ts | 57 +++++++++++++++++- .../test/unit/services/SegmentService.test.ts | 30 ++++------ 8 files changed, 216 insertions(+), 90 deletions(-) delete mode 100644 packages/backend/src/database/migrations/1782950400000-clearFeatureFlagPrecomputedSegment.ts diff --git a/packages/backend/CLAUDE.md b/packages/backend/CLAUDE.md index 78d95af35b..3d3ccc7097 100644 --- a/packages/backend/CLAUDE.md +++ b/packages/backend/CLAUDE.md @@ -83,9 +83,11 @@ Segment inclusion/exclusion for **feature flags** is precomputed and stored flat - **`FeatureFlagPrecomputedSegment` entity** (`src/api/models/FeatureFlagPrecomputedSegment.ts`) — one row per feature flag, columns: `featureFlagId` (PK), `inclusionIds: text[]`, `exclusionIds: text[]`. FK to `feature_flag` with `onDelete: CASCADE`. - **`FeatureFlagPrecomputedSegmentService`** (`src/api/services/FeatureFlagPrecomputedSegmentService.ts`) — owns all computation and cache logic: - - `recomputeForFlag(flagId)` — flattens all enabled inclusion/exclusion segments (recursive sub-segments) into flat ID arrays and upserts the row. - - `scheduleRecomputeForSegment(segmentId)` — fire-and-forget; finds all flags referencing a segment (and its parents) and calls `recomputeForFlag` for each. - - `getAffectedFlagIds(segmentId)` — public helper that returns flag IDs affected by a given segment (used before deletion). + - `recomputeForFlag(flagId)` — flattens all enabled inclusion/exclusion segments (recursive sub-segments) into flat ID arrays and upserts the row. This is the only method that `await`s — callers on write paths never call it directly. + - `scheduleRecomputeForFlags(flagIds[])` — fire-and-forget recompute for a known set of flags (swallows/logs errors). The flag-side counterpart to `scheduleRecomputeForSegment`. + - `scheduleRecomputeForSegment(segmentId)` — fire-and-forget; finds all flags referencing a segment (and its parents) and recomputes each. + - `withRecompute(logger, resolveAffectedFlagIds, work)` — **the wrapper all top-level write methods use.** Resolves affected flag IDs *before* `work`, runs `work` (which must own/commit its own transaction), then fires a fire-and-forget recompute *after* commit. Keeps mutation + recompute in one call so a refactor can't drop the recompute. `work` is never blocked on the recompute. + - `getAffectedFlagIds(segmentId)` — public helper that returns flag IDs affected by a given segment (used as the `resolveAffectedFlagIds` for segment deletes). - `getPrecomputedSets(flagIds[])` — cache-wrapped batch fetch, returns a `Map`. - `backfillMissingFlags(logger)` — called at startup; computes rows only for flags that have none yet (no-op once all flags are populated). - `recomputeAllFlags(logger)` — full refresh of every flag; not called automatically, available for manual recovery. @@ -96,17 +98,18 @@ Segment inclusion/exclusion for **feature flags** is precomputed and stored flat | Event | Trigger | |---|---| -| Segment list added to a flag | `FeatureFlagService.addList` → `recomputeForFlag` | -| Segment list removed from a flag | `FeatureFlagService.deleteList` → `recomputeForFlag` | -| Segment list members updated on a flag | `FeatureFlagService.updateList` → `recomputeForFlag` | +| Segment list added to a flag | `FeatureFlagService.addList` → `withRecompute` | +| Segment list removed from a flag | `FeatureFlagService.deleteList` → delegates to `SegmentService.deleteSegment` (which owns the recompute) | +| Segment list members updated on a flag | `FeatureFlagService.updateList` → `withRecompute` | +| Flag context changed (deletes all its lists) | `FeatureFlagService.updateFeatureFlagInDB` → `withRecompute` (recomputes to empty; the segment delete does **not** cascade to the precomputed row) | | Private list added to a shared segment | `SegmentService.addList` → `scheduleRecomputeForSegment` | | Private list removed from a shared segment | `SegmentService.deleteList` → `scheduleRecomputeForSegment` | | Segment members/structure updated | `SegmentService.addSegmentDataWithPipeline` → `scheduleRecomputeForSegment` | -| Segment deleted entirely | `SegmentService.deleteSegment` — collects affected flag IDs **before** deletion, fires `recomputeForFlag` for each **after** deletion (fire-and-forget) | +| Segment deleted entirely | `SegmentService.deleteSegment` → `withRecompute` (collects affected flag IDs **before** the delete, recomputes **after** commit) | | Server startup | `app.ts` → `backfillMissingFlags` — backfills any flag with no row | -All recomputes triggered from write paths are **fire-and-forget** — callers never wait on them. +All recomputes triggered from write paths are **fire-and-forget** — no request handler (flag-side or segment-side) ever blocks on a recompute. The `import*` paths are the one exception: they `await recomputeForFlag` so "import complete" means the rows are ready. ### Key invariant -The `feature_flag_precomputed_segment` row must always be recomputed **after** the structural change completes, so the flat arrays reflect the new state. For deletions specifically, affected flag IDs must be collected **before** the delete because the join table records are gone afterward. +The `feature_flag_precomputed_segment` row must always be recomputed **after** the structural change commits, so the flat arrays reflect the new state. For deletions specifically, affected flag IDs must be collected **before** the delete because the join table records are gone afterward. Both halves of this invariant are enforced by `withRecompute` (resolve-before → work → recompute-after), so top-level write methods get the ordering for free rather than hand-rolling it. diff --git a/packages/backend/src/api/services/FeatureFlagPrecomputedSegmentService.ts b/packages/backend/src/api/services/FeatureFlagPrecomputedSegmentService.ts index 635cacb0aa..621561eb9b 100644 --- a/packages/backend/src/api/services/FeatureFlagPrecomputedSegmentService.ts +++ b/packages/backend/src/api/services/FeatureFlagPrecomputedSegmentService.ts @@ -86,6 +86,42 @@ export class FeatureFlagPrecomputedSegmentService { .catch((err) => logger.error({ message: `Error in scheduleRecomputeForSegment: ${err}` })); } + // Fire-and-forget recompute for a known set of flags — the flag-side counterpart to + // scheduleRecomputeForSegment. Callers on the write path MUST NOT await this: the recompute + // is a read-through cache refresh that can run after the response is returned. Errors are + // swallowed (logged) so an unhandled rejection can never crash the process. + public scheduleRecomputeForFlags(flagIds: string[], logger: UpgradeLogger): void { + Promise.all([...new Set(flagIds)].map((flagId) => this.recomputeForFlag(flagId, logger))).catch((err) => + logger.error({ message: `Error in scheduleRecomputeForFlags: ${err}` }) + ); + } + + // Run a segment/flag-list mutation and guarantee the affected flags' precomputed rows are + // refreshed afterward — without the caller ever awaiting (or having to remember) the recompute. + // + // The ordering contract is enforced here, once, so individual write methods can't get it wrong + // during a later refactor: + // 1. `resolveAffectedFlagIds` runs BEFORE `work`. Required for deletes (once the join rows are + // gone we can no longer discover which flags referenced the segment) and harmless for + // adds/updates (the flags are already known / already attached). + // 2. `work` runs to completion. It MUST own and commit its own transaction: the recompute reads + // through this service's own repositories and cannot see a still-open transaction's writes. + // 3. The recompute is fired fire-and-forget AFTER `work` resolves (post-commit) and is NOT + // awaited, so the HTTP response is never blocked on it. + // + // Because the mutation and its recompute live in a single call, a change to the mutation body + // can't silently drop the recompute — the two can't drift apart. + public async withRecompute( + logger: UpgradeLogger, + resolveAffectedFlagIds: () => string[] | Promise, + work: () => Promise + ): Promise { + const affectedFlagIds = await resolveAffectedFlagIds(); + const result = await work(); + this.scheduleRecomputeForFlags(affectedFlagIds, logger); + return result; + } + public async getPrecomputedSets(flagIds: string[]): Promise> { if (!flagIds.length) return new Map(); diff --git a/packages/backend/src/api/services/FeatureFlagService.ts b/packages/backend/src/api/services/FeatureFlagService.ts index b896e38df9..b4f3c3fd39 100644 --- a/packages/backend/src/api/services/FeatureFlagService.ts +++ b/packages/backend/src/api/services/FeatureFlagService.ts @@ -447,7 +447,13 @@ export class FeatureFlagService { const includeListIds = includeList.map((list) => list.segment.id); const excludeListIds = excludeList.map((list) => list.segment.id); - return await this.dataSource.transaction(async (transactionalEntityManager) => { + // A context change (below) deletes all of this flag's inclusion/exclusion lists. That delete does + // NOT cascade to feature_flag_precomputed_segment (its FK is to feature_flag, not segment), so the + // row would otherwise keep stale member IDs. Recompute it (to empty) after commit via withRecompute. + // Non-context updates don't touch lists, so there is nothing to recompute. + const contextChanged = oldFlagDoc.context[0] !== flag.context[0]; + + const applyUpdate = async (transactionalEntityManager: EntityManager) => { const { featureFlagSegmentExclusion, featureFlagSegmentInclusion, @@ -512,7 +518,13 @@ export class FeatureFlagService { featureFlagSegmentInclusion: includeList, featureFlagSegmentExclusion: excludeList, }; - }); + }; + + return this.featureFlagPrecomputedSegmentService.withRecompute( + logger, + () => (contextChanged ? [flag.id] : []), + () => this.dataSource.transaction(applyUpdate) + ); } public async deleteList( @@ -521,23 +533,12 @@ export class FeatureFlagService { currentUser: UserDTO, logger: UpgradeLogger ): Promise { - // Capture the flag id before deletion for recompute - const repo = - filterType === LIST_FILTER_MODE.INCLUSION - ? this.featureFlagSegmentInclusionRepository - : this.featureFlagSegmentExclusionRepository; - const record = await repo.findOne({ where: { segment: { id: segmentId } }, relations: ['featureFlag'] }); - const flagId = record?.featureFlag?.id; - await this.createDeleteListAuditLogs([segmentId], filterType, currentUser); await this.cacheService.resetPrefixCache(CACHE_PREFIX.FEATURE_FLAG_KEY_PREFIX); - const deleted = await this.segmentService.deleteSegment(segmentId, logger); - if (flagId) { - await this.featureFlagPrecomputedSegmentService.recomputeForFlag(flagId, logger); - } - - return deleted; + // segmentService.deleteSegment collects the affected flags before deletion and fires the + // fire-and-forget recompute itself (via withRecompute), so no separate recompute is needed here. + return this.segmentService.deleteSegment(segmentId, logger); } async createDeleteListAuditLogs( @@ -691,14 +692,12 @@ export class FeatureFlagService { // caller is responsible for calling recomputeForFlag after its transaction commits. result = await executeTransaction(transactionalEntityManager); } else { - result = await this.dataSource.transaction(async (manager) => { - return await executeTransaction(manager); - }); - - // Recompute precomputed sets for each affected flag after the transaction commits - const affectedFlagIds = [...new Set(listsInput.map((l) => l.id))]; - await Promise.all( - affectedFlagIds.map((flagId) => this.featureFlagPrecomputedSegmentService.recomputeForFlag(flagId, logger)) + // withRecompute runs the mutation in its own transaction, then fires a fire-and-forget + // recompute for the affected flags after commit — the caller never awaits it. + result = await this.featureFlagPrecomputedSegmentService.withRecompute( + logger, + () => [...new Set(listsInput.map((l) => l.id))], + () => this.dataSource.transaction((manager) => executeTransaction(manager)) ); } @@ -743,7 +742,7 @@ export class FeatureFlagService { ): Promise { logger.info({ message: `Update ${filterType} list for feature flag` }); await this.cacheService.resetPrefixCache(CACHE_PREFIX.FEATURE_FLAG_KEY_PREFIX); - const result = await this.dataSource.transaction(async (transactionalEntityManager) => { + const doUpdate = async (transactionalEntityManager: EntityManager) => { // Find the existing record let existingRecord: FeatureFlagSegmentInclusion | FeatureFlagSegmentExclusion; const featureFlag = await this.findOne(listInput.id); @@ -851,9 +850,15 @@ export class FeatureFlagService { await this.experimentAuditLogRepository.saveRawJson(LOG_TYPE.FEATURE_FLAG_UPDATED, updateAuditLog, currentUser); return existingRecord; - }); + }; - await this.featureFlagPrecomputedSegmentService.recomputeForFlag(listInput.id, logger); + // withRecompute runs the update transaction, then fires a fire-and-forget recompute for the + // affected flag after commit — the caller never awaits it. + const result = await this.featureFlagPrecomputedSegmentService.withRecompute( + logger, + () => [listInput.id], + () => this.dataSource.transaction(doUpdate) + ); return result; } diff --git a/packages/backend/src/api/services/SegmentService.ts b/packages/backend/src/api/services/SegmentService.ts index 90e0aaa937..6654206cf5 100644 --- a/packages/backend/src/api/services/SegmentService.ts +++ b/packages/backend/src/api/services/SegmentService.ts @@ -479,21 +479,17 @@ export class SegmentService { public async deleteSegment(id: string, logger: UpgradeLogger): Promise { logger.info({ message: `Delete segment by id. segmentId: ${id}` }); - // Collect affected flags before deletion — join table records are gone after - const affectedFlagIds = await this.featureFlagPrecomputedSegmentService.getAffectedFlagIds(id); - const manager = this.dataSource; - const deletedSegment = await manager.transaction(async (transactionalEntityManager) => { - return this.deleteSegmentAndPrivateSubsegments(id, logger, transactionalEntityManager); - }); - - // Recompute after the delete transaction has committed so the recompute reads the - // post-delete state and stale member IDs are removed (fire-and-forget). - affectedFlagIds.forEach((flagId) => - this.featureFlagPrecomputedSegmentService - .recomputeForFlag(flagId, logger) - .catch((err) => - logger.error({ message: `Error recomputing feature_flag_precomputed_segment for flag ${flagId}`, error: err }) + // withRecompute collects the affected flags BEFORE the delete (the join rows are gone after), + // runs the delete in its own transaction, then fires a fire-and-forget recompute for those + // flags after commit. Keeping the ordering inside the wrapper means a future refactor of this + // method can't accidentally break it. + const deletedSegment = await this.featureFlagPrecomputedSegmentService.withRecompute( + logger, + () => this.featureFlagPrecomputedSegmentService.getAffectedFlagIds(id), + () => + this.dataSource.transaction((transactionalEntityManager) => + this.deleteSegmentAndPrivateSubsegments(id, logger, transactionalEntityManager) ) ); diff --git a/packages/backend/src/database/migrations/1782950400000-clearFeatureFlagPrecomputedSegment.ts b/packages/backend/src/database/migrations/1782950400000-clearFeatureFlagPrecomputedSegment.ts deleted file mode 100644 index 963d397b3d..0000000000 --- a/packages/backend/src/database/migrations/1782950400000-clearFeatureFlagPrecomputedSegment.ts +++ /dev/null @@ -1,20 +0,0 @@ -import { MigrationInterface, QueryRunner } from 'typeorm'; - -// The stored format of group IDs in feature_flag_precomputed_segment changed: group IDs are now -// namespaced with their group type (see precomputedGroupKey) instead of stored bare. Existing rows -// hold the old bare format and would silently stop matching, so clear the table. The startup -// backfill (backfillMissingFlags) then rebuilds every row in the new format. Until backfill -// completes, flags with a missing row fall back to on-the-fly resolution, so no wrong decisions are -// made in the interim. -export class ClearFeatureFlagPrecomputedSegment1782950400000 implements MigrationInterface { - name = 'ClearFeatureFlagPrecomputedSegment1782950400000'; - - public async up(queryRunner: QueryRunner): Promise { - await queryRunner.query(`DELETE FROM "feature_flag_precomputed_segment"`); - } - - public async down(queryRunner: QueryRunner): Promise { - // Reverting the code reverts the stored format too; clear again so the (old-format) backfill rebuilds. - await queryRunner.query(`DELETE FROM "feature_flag_precomputed_segment"`); - } -} diff --git a/packages/backend/test/unit/services/FeatureFlagPrecomputedSegmentService.test.ts b/packages/backend/test/unit/services/FeatureFlagPrecomputedSegmentService.test.ts index 8c406790f1..3eeb8f2f60 100644 --- a/packages/backend/test/unit/services/FeatureFlagPrecomputedSegmentService.test.ts +++ b/packages/backend/test/unit/services/FeatureFlagPrecomputedSegmentService.test.ts @@ -209,4 +209,63 @@ describe('FeatureFlagPrecomputedSegmentService', () => { expect(recomputeSpy).toHaveBeenCalledWith('f2', logger); }); }); + + describe('withRecompute', () => { + it('resolves affected flag ids BEFORE running work, returns work result, and recomputes after', async () => { + const order: string[] = []; + const resolveAffectedFlagIds = jest.fn(async () => { + order.push('resolve'); + return ['f1']; + }); + const work = jest.fn(async () => { + order.push('work'); + return 'done'; + }); + + const result = await service.withRecompute(logger, resolveAffectedFlagIds, work); + + expect(result).toBe('done'); + expect(order).toEqual(['resolve', 'work']); // resolve strictly before the mutation + expect(resolveAffectedFlagIds).toHaveBeenCalledTimes(1); + expect(work).toHaveBeenCalledTimes(1); + + // the recompute is fired after work; let the fire-and-forget chain settle + await new Promise((r) => setImmediate(r)); + expect(precomputedSegmentRepository.upsertByFlagId).toHaveBeenCalledWith('f1', [], []); + }); + + it('does not await the recompute — resolves even if the recompute never settles', async () => { + // upsertByFlagId never resolves => recomputeForFlag never settles. If withRecompute awaited + // the recompute, this would hang and time out. + precomputedSegmentRepository.upsertByFlagId = jest.fn(() => new Promise(() => undefined)); + + await expect( + service.withRecompute( + logger, + () => ['f1'], + async () => 'ok' + ) + ).resolves.toBe('ok'); + }); + + it('still resolves (and logs) when the fire-and-forget recompute fails', async () => { + precomputedSegmentRepository.upsertByFlagId = jest.fn().mockRejectedValue(new Error('recompute boom')); + const errorSpy = jest.spyOn(logger, 'error'); + + await expect( + service.withRecompute( + logger, + () => ['f1'], + async () => 'ok' + ) + ).resolves.toBe('ok'); + + // let the fire-and-forget .catch settle so the rejection is handled (no unhandled rejection) + await new Promise((r) => setImmediate(r)); + expect(errorSpy).toHaveBeenCalledWith( + expect.objectContaining({ message: expect.stringContaining('scheduleRecomputeForFlags') }) + ); + errorSpy.mockRestore(); + }); + }); }); diff --git a/packages/backend/test/unit/services/FeatureFlagService.test.ts b/packages/backend/test/unit/services/FeatureFlagService.test.ts index 887928502f..e13321354d 100644 --- a/packages/backend/test/unit/services/FeatureFlagService.test.ts +++ b/packages/backend/test/unit/services/FeatureFlagService.test.ts @@ -214,6 +214,13 @@ describe('Feature Flag Service Testing', () => { recomputeForFlag: jest.fn().mockResolvedValue(undefined), seedEmptyRowForFlag: jest.fn().mockResolvedValue(undefined), scheduleRecomputeForSegment: jest.fn(), + scheduleRecomputeForFlags: jest.fn(), + // Faithful stub: run the resolver + work so the mutation still executes; the real + // wrapper's fire-and-forget recompute behavior is covered in the precompute service's suite. + withRecompute: jest.fn(async (_logger, resolveAffectedFlagIds, work) => { + await resolveAffectedFlagIds(); + return work(); + }), getAffectedFlagIds: jest.fn().mockResolvedValue([]), }, }, @@ -459,6 +466,49 @@ describe('Feature Flag Service Testing', () => { ); }); + it('recomputes the precomputed row when a flag update changes its context (lists are deleted)', async () => { + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); + // old flag has a different context than the incoming mockFlag2 (context: ['context']) and has lists + service.findOne = jest.fn().mockResolvedValue({ + id: mockFlag2.id, + name: 'name', + key: 'key', + description: 'description', + context: ['old-context'], + status: FEATURE_FLAG_STATUS.ENABLED, + featureFlagSegmentInclusion: [{ segment: { id: 'inc-seg' } }], + featureFlagSegmentExclusion: [{ segment: { id: 'exc-seg' } }], + }); + + await service.update(mockFlag2, mockUser1, logger); + + expect(precomputed.withRecompute).toHaveBeenCalled(); + // the resolver targets this flag so its (now empty) row is rebuilt instead of left stale + const [, resolveAffectedFlagIds] = (precomputed.withRecompute as jest.Mock).mock.calls[0]; + expect(await resolveAffectedFlagIds()).toEqual([mockFlag2.id]); + }); + + it('does not recompute the precomputed row when a flag update leaves the context unchanged', async () => { + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); + // old flag has the SAME context as the incoming mockFlag2 (context: ['context']) — lists untouched + service.findOne = jest.fn().mockResolvedValue({ + id: mockFlag2.id, + name: 'name', + key: 'key', + description: 'description', + context: ['context'], + status: FEATURE_FLAG_STATUS.ENABLED, + featureFlagSegmentInclusion: [], + featureFlagSegmentExclusion: [], + }); + + await service.update(mockFlag2, mockUser1, logger); + + // withRecompute still wraps the write, but its resolver yields no flags => no recompute fired + const [, resolveAffectedFlagIds] = (precomputed.withRecompute as jest.Mock).mock.calls[0]; + expect(await resolveAffectedFlagIds()).toEqual([]); + }); + it('should update the flag state', async () => { const results = await service.updateState(mockFlag1.id, FEATURE_FLAG_STATUS.ENABLED, mockUser1); expect(results).toBeTruthy(); @@ -799,12 +849,15 @@ describe('Feature Flag Service Testing', () => { expect(precomputed.seedEmptyRowForFlag).toHaveBeenCalledWith(mockFlag1.id, expect.anything()); }); - it('recomputes the affected flag after addList (standalone, owns the transaction)', async () => { + it('recomputes the affected flag after addList (standalone) via withRecompute', async () => { const precomputed = module.get(FeatureFlagPrecomputedSegmentService); await service.addList([mockList], LIST_FILTER_MODE.INCLUSION, mockUser1, logger); - expect(precomputed.recomputeForFlag).toHaveBeenCalledWith(mockList.id, logger); + expect(precomputed.withRecompute).toHaveBeenCalled(); + // the resolver handed to withRecompute yields the affected flag id + const [, resolveAffectedFlagIds] = (precomputed.withRecompute as jest.Mock).mock.calls[0]; + expect(await resolveAffectedFlagIds()).toEqual([mockList.id]); }); it('recomputes imported flags after the import transaction commits', async () => { diff --git a/packages/backend/test/unit/services/SegmentService.test.ts b/packages/backend/test/unit/services/SegmentService.test.ts index 4df30115a4..65f8367988 100644 --- a/packages/backend/test/unit/services/SegmentService.test.ts +++ b/packages/backend/test/unit/services/SegmentService.test.ts @@ -200,10 +200,17 @@ describe('Segment Service Testing', () => { provide: FeatureFlagPrecomputedSegmentService, useValue: { scheduleRecomputeForSegment: jest.fn(), + scheduleRecomputeForFlags: jest.fn(), recomputeForFlag: jest.fn().mockResolvedValue(undefined), getAffectedFlagIds: jest.fn().mockResolvedValue([]), seedEmptyRowForFlag: jest.fn().mockResolvedValue(undefined), getPrecomputedSets: jest.fn().mockResolvedValue(new Map()), + // Faithful stub: run the resolver + work so the mutation still executes; the real + // wrapper's fire-and-forget recompute behavior is covered in the precompute service's suite. + withRecompute: jest.fn(async (_logger, resolveAffectedFlagIds, work) => { + await resolveAffectedFlagIds(); + return work(); + }), }, }, { @@ -748,30 +755,17 @@ describe('Segment Service Testing', () => { }); describe('precomputed segment recompute triggers', () => { - it('collects affected flags before deleting and recomputes them after the delete commits', async () => { + it('delegates the collect-before-mutate-then-recompute ordering for deleteSegment to withRecompute', async () => { const precomputed = module.get(FeatureFlagPrecomputedSegmentService); (precomputed.getAffectedFlagIds as jest.Mock).mockResolvedValue(['flagA']); await service.deleteSegment(seg1.id, logger); + expect(precomputed.withRecompute).toHaveBeenCalled(); + // the resolver handed to withRecompute collects the affected flags for this segment + const [, resolveAffectedFlagIds] = (precomputed.withRecompute as jest.Mock).mock.calls[0]; + await expect(resolveAffectedFlagIds()).resolves.toEqual(['flagA']); expect(precomputed.getAffectedFlagIds).toHaveBeenCalledWith(seg1.id); - expect(precomputed.recomputeForFlag).toHaveBeenCalledWith('flagA', logger); - }); - - it('logs and does not reject deleteSegment when a fire-and-forget recompute fails', async () => { - const precomputed = module.get(FeatureFlagPrecomputedSegmentService); - (precomputed.getAffectedFlagIds as jest.Mock).mockResolvedValue(['flagA']); - (precomputed.recomputeForFlag as jest.Mock).mockRejectedValueOnce(new Error('recompute boom')); - const errorSpy = jest.spyOn(logger, 'error'); - - // deleteSegment must still resolve — the recompute is fire-and-forget - await expect(service.deleteSegment(seg1.id, logger)).resolves.toBeDefined(); - - // let the fire-and-forget .catch settle so the rejection is handled (no unhandled rejection) - await new Promise((resolve) => setImmediate(resolve)); - - expect(errorSpy).toHaveBeenCalledWith(expect.objectContaining({ message: expect.stringContaining('flagA') })); - errorSpy.mockRestore(); }); it('schedules a recompute when a list is added to a segment', async () => { From 6cb6080080a5082e2ec6f1987f85507e23175b54 Mon Sep 17 00:00:00 2001 From: doswalt Date: Mon, 6 Jul 2026 11:22:21 -0400 Subject: [PATCH 14/16] block accidental save while members loading --- packages/backend/src/api/models/Segment.ts | 4 +- .../src/api/services/FeatureFlagService.ts | 21 +++----- .../src/api/services/SegmentService.ts | 11 ++-- .../unit/services/FeatureFlagService.test.ts | 10 +++- .../core/segments/segments.data.service.ts | 4 +- .../src/app/core/segments/segments.service.ts | 3 +- .../app/core/segments/store/segments.model.ts | 3 +- ...rt-private-segment-list-modal.component.ts | 52 +++++++++++++------ ...etails-participant-list-table.component.ts | 3 +- 9 files changed, 59 insertions(+), 52 deletions(-) diff --git a/packages/backend/src/api/models/Segment.ts b/packages/backend/src/api/models/Segment.ts index a8131023f9..c7f098225a 100644 --- a/packages/backend/src/api/models/Segment.ts +++ b/packages/backend/src/api/models/Segment.ts @@ -48,9 +48,7 @@ export class Segment extends BaseModel { @Type(() => GroupForSegment) public groupForSegment: GroupForSegment[]; - // Not persisted columns. Populated via loadRelationCountAndMap when a segment is loaded - // without its member lists (e.g. the feature-flag details page), so the UI can show counts - // without shipping the full individualForSegment / groupForSegment arrays. + // Not persisted; populated via loadRelationCountAndMap for counts-only loads (see findOneForDetails). public individualForSegmentCount?: number; public groupForSegmentCount?: number; diff --git a/packages/backend/src/api/services/FeatureFlagService.ts b/packages/backend/src/api/services/FeatureFlagService.ts index 299e1be182..a6182ca435 100644 --- a/packages/backend/src/api/services/FeatureFlagService.ts +++ b/packages/backend/src/api/services/FeatureFlagService.ts @@ -155,13 +155,8 @@ export class FeatureFlagService { return featureFlag; } - // Lightweight variant of findOne for the details page, which only renders member *counts* - // per inclusion/exclusion list. It skips loading the (potentially tens of thousands of rows) - // individualForSegment and groupForSegment collections — which in findOne also cause a - // Cartesian-product row explosion — and instead maps lightweight COUNT subqueries onto each - // segment (individualForSegmentCount / groupForSegmentCount). The full member lists are - // fetched on demand via GET /segments/:id/members when a list is opened for editing. - // NOTE: callers that need the actual members (e.g. exports) must use findOne, not this. + // Counts-only variant of findOne for the details page: maps member counts instead of loading + // the member lists. Callers that need the actual members (e.g. exports) must use findOne. public async findOneForDetails(id: string, logger?: UpgradeLogger): Promise { if (logger) { logger.info({ message: `Find feature flag (details view) by id => ${id}` }); @@ -273,7 +268,7 @@ export class FeatureFlagService { ): Promise { logger.info({ message: `Delete Feature Flag => ${featureFlagId}` }); return await this.dataSource.transaction(async (transactionalEntityManager) => { - const featureFlag = await this.findOne(featureFlagId, logger); + const featureFlag = await this.findOneForDetails(featureFlagId, logger); if (featureFlag) { await this.clearCachedFlagsForContext(featureFlag.context[0]); @@ -317,7 +312,7 @@ export class FeatureFlagService { } public async updateState(flagId: string, status: FEATURE_FLAG_STATUS, currentUser: UserDTO): Promise { - const oldFeatureFlag = await this.findOne(flagId); + const oldFeatureFlag = await this.findOneForDetails(flagId); await this.clearCachedFlagsForContext(oldFeatureFlag.context[0]); let updatedState: FeatureFlag; try { @@ -447,7 +442,7 @@ export class FeatureFlagService { createdAt, updatedAt, ...oldFlagDoc - } = await this.findOne(flag.id); + } = await this.findOneForDetails(flag.id); let includeList = [...featureFlagSegmentInclusion]; let excludeList = [...featureFlagSegmentExclusion]; @@ -727,9 +722,7 @@ export class FeatureFlagService { logger.info({ message: `Update ${filterType} list for feature flag` }); await this.cacheService.resetPrefixCache(CACHE_PREFIX.FEATURE_FLAG_KEY_PREFIX); return await this.dataSource.transaction(async (transactionalEntityManager) => { - // Find the existing record. Only the flag id/name are needed here (for the audit log - // below), so use the counts-only variant to avoid loading every list's members — which - // for large lists made saving an edit very slow. + // Only the flag id/name are needed here (audit log), so use the counts-only variant. let existingRecord: FeatureFlagSegmentInclusion | FeatureFlagSegmentExclusion; const featureFlag = await this.findOneForDetails(listInput.id); @@ -1328,7 +1321,7 @@ export class FeatureFlagService { const featureFlagListFile = featureFlagListFiles.find((file) => file.fileName === fileStatus.fileName); return this.segmentService.convertJSONStringToSegInputValFormat(featureFlagListFile.fileContent as string); }); - const featureFlag = await this.findOne(featureFlagId, logger); + const featureFlag = await this.findOneForDetails(featureFlagId, logger); const createdLists: (FeatureFlagSegmentInclusion | FeatureFlagSegmentExclusion)[] = await this.dataSource.transaction(async (transactionalEntityManager) => { diff --git a/packages/backend/src/api/services/SegmentService.ts b/packages/backend/src/api/services/SegmentService.ts index 40273c9ecb..343c5d28f4 100644 --- a/packages/backend/src/api/services/SegmentService.ts +++ b/packages/backend/src/api/services/SegmentService.ts @@ -150,10 +150,7 @@ export class SegmentService { return segmentDoc; } - // Fetches a segment (including private lists) by id with its full member lists. Unlike - // getSegmentById this does not exclude private segments, so a feature flag or experiment's - // private inclusion/exclusion list can be loaded on demand for editing (its members are not - // loaded on the counts-only details page). + // Like getSegmentById but includes private lists, so a flag/experiment list can be loaded for editing. public async getSegmentByIdWithMembers(id: string, logger: UpgradeLogger): Promise { logger.info({ message: `Find segment (including private) with members by id. segmentId: ${id}` }); return this.segmentRepository @@ -901,10 +898,8 @@ export class SegmentService { if (segment.id) { try { - // Clear the existing members before re-inserting them below (the update model is a full - // replace). We delete with a single "WHERE segmentId = :id" statement per member table. - // Previously this fetched every member and passed a per-row criteria array to .delete(), - // which TypeORM expands into one giant OR predicate — extremely slow for large lists. + // Full replace: clear members with one delete per table. A per-row criteria array (the + // previous approach) expands into a giant OR predicate that is very slow for large lists. await Promise.all([ transactionalEntityManager.getRepository(IndividualForSegment).delete({ segmentId: segment.id }), transactionalEntityManager.getRepository(GroupForSegment).delete({ segmentId: segment.id }), diff --git a/packages/backend/test/unit/services/FeatureFlagService.test.ts b/packages/backend/test/unit/services/FeatureFlagService.test.ts index c9af84b4a9..c06a8923a2 100644 --- a/packages/backend/test/unit/services/FeatureFlagService.test.ts +++ b/packages/backend/test/unit/services/FeatureFlagService.test.ts @@ -448,6 +448,14 @@ describe('Feature Flag Service Testing', () => { expect(results).toBeTruthy(); }); + it('should use the counts-only fetch (not the full-member findOne) when toggling flag state', async () => { + const detailsSpy = jest.spyOn(service, 'findOneForDetails'); + const findOneSpy = jest.spyOn(service, 'findOne'); + await service.updateState(mockFlag1.id, FEATURE_FLAG_STATUS.ENABLED, mockUser1); + expect(detailsSpy).toHaveBeenCalledWith(mockFlag1.id); + expect(findOneSpy).not.toHaveBeenCalled(); + }); + it('should update the filter mode', async () => { flagRepo.updateFilterMode = jest.fn().mockResolvedValue(mockFlag1); const results = await service.updateFilterMode(mockFlag1.id, FILTER_MODE.EXCLUDE_ALL, mockUser1); @@ -461,7 +469,7 @@ describe('Feature Flag Service Testing', () => { }); it('should return undefined when no flag to delete', async () => { - service.findOne = jest.fn().mockResolvedValue(undefined); + service.findOneForDetails = jest.fn().mockResolvedValue(undefined); const results = await service.delete(mockFlag1.id, mockUser1, logger); expect(results).toEqual(undefined); }); diff --git a/packages/frontend/projects/upgrade/src/app/core/segments/segments.data.service.ts b/packages/frontend/projects/upgrade/src/app/core/segments/segments.data.service.ts index 714d3dc449..c533fb3d5f 100644 --- a/packages/frontend/projects/upgrade/src/app/core/segments/segments.data.service.ts +++ b/packages/frontend/projects/upgrade/src/app/core/segments/segments.data.service.ts @@ -48,9 +48,7 @@ export class SegmentsDataService { return this.http.get(url); } - // Fetches a single segment including its full member lists. Used to lazy-load members when - // editing a list, since the details page loads segments with counts only (no member arrays). - // Uses the /members endpoint, which (unlike GET /segments/:id) also returns private lists. + // Lazy-loads a list's members for editing; the /members endpoint also returns private lists. fetchSegmentWithMembersById(id: string): Observable { const url = `${API_ENDPOINTS.segments}/${id}/members`; return this.http.get(url); diff --git a/packages/frontend/projects/upgrade/src/app/core/segments/segments.service.ts b/packages/frontend/projects/upgrade/src/app/core/segments/segments.service.ts index d2e1d98ef8..12dd838630 100644 --- a/packages/frontend/projects/upgrade/src/app/core/segments/segments.service.ts +++ b/packages/frontend/projects/upgrade/src/app/core/segments/segments.service.ts @@ -123,8 +123,7 @@ export class SegmentsService { this.store$.dispatch(SegmentsActions.actionGetSegmentById({ segmentId })); } - // Fetches a single segment with its full member lists directly (bypassing the store), used - // to lazy-load members when editing a list loaded from the counts-only details endpoint. + // Lazy-loads a list's members for editing (bypasses the store). fetchSegmentWithMembersById(segmentId: string): Observable { return this.segmentsDataService.fetchSegmentWithMembersById(segmentId); } diff --git a/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.model.ts b/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.model.ts index 82d43a61aa..bb36019ebb 100644 --- a/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.model.ts +++ b/packages/frontend/projects/upgrade/src/app/core/segments/store/segments.model.ts @@ -107,8 +107,7 @@ export interface Segment { individualForSegment: IndividualForSegment[]; groupForSegment: GroupForSegment[]; subSegments: Segment[]; - // Lightweight member counts returned when the segment is loaded without its full member - // lists (e.g. the feature-flag details page). Undefined when the full lists are present. + // Member counts from counts-only loads (e.g. flag details); undefined when full lists are present. individualForSegmentCount?: number; groupForSegmentCount?: number; listType?: MemberTypes | string; diff --git a/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-private-segment-list-modal/upsert-private-segment-list-modal.component.ts b/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-private-segment-list-modal/upsert-private-segment-list-modal.component.ts index 9427ccfa5f..c1fccbf556 100644 --- a/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-private-segment-list-modal/upsert-private-segment-list-modal.component.ts +++ b/packages/frontend/projects/upgrade/src/app/features/dashboard/segments/modals/upsert-private-segment-list-modal/upsert-private-segment-list-modal.component.ts @@ -37,8 +37,10 @@ import { import { MatAutocompleteModule } from '@angular/material/autocomplete'; import { BehaviorSubject, + catchError, combineLatest, combineLatestWith, + EMPTY, map, Observable, startWith, @@ -72,15 +74,18 @@ import { SharedModule } from '../../../../../shared/shared.module'; export class UpsertPrivateSegmentListModalComponent { @ViewChild('typeSelectRef') typeSelectRef: MatSelect; listOptionTypes$: Observable<{ value: string; viewValue: string }[]>; - // This modal drives add/edit for feature-flag lists, experiment lists, and standalone - // segment lists — each backed by a different store. Disable the primary button while an - // upsert is in flight in ANY of them so it can't be double-submitted. Each store resets its - // own flag on success/failure, so this re-enables (or the modal closes) automatically. + // Disable the primary button while an add/edit is in flight in any of the three stores this + // modal drives (flag/experiment/segment), to prevent double-submits. isUpsertLoading$ = combineLatest([ this.featureFlagService.isLoadingUpsertPrivateSegmentList$, this.experimentService.isLoadingUpsertPrivateSegmentList$, this.segmentsService.isLoadingSegments$, ]).pipe(map((loadingFlags) => loadingFlags.some(Boolean))); + // True while the lazy member fetch (for counts-only edit sources) is in flight. Until it + // resolves the form's values control holds only the partial (counts-only) data, so submitting + // would send a full-replacement update that drops the unloaded members. Included in + // isPrimaryButtonDisabled$ to block saving during the fetch. + isLoadingMembers$ = new BehaviorSubject(false); initialFormValues$ = new BehaviorSubject(null); subscriptions = new Subscription(); @@ -207,17 +212,28 @@ export class UpsertPrivateSegmentListModalComponent { this.applyEditFormValues(sourceList.listType, sourceList.segment); - // The feature-flag details page loads segments with member counts only (no member - // arrays) to stay lightweight, so lazy-load the full segment when we detect that members - // exist but weren't loaded. Editing then operates on the complete list. + // Lazy-load the full members when the (counts-only) source list didn't include them. if (this.segmentMembersNeedFetch(sourceList.listType, sourceList.segment)) { + // Block saving until the members load; otherwise a submit could full-replace with partial data. + this.isLoadingMembers$.next(true); this.subscriptions.add( - this.segmentsService.fetchSegmentWithMembersById(sourceList.segment.id).subscribe((segment) => { - if (segment) { - this.applyEditFormValues(sourceList.listType, segment); - this.changeDetectorRef.markForCheck(); - } - }) + this.segmentsService + .fetchSegmentWithMembersById(sourceList.segment.id) + .pipe( + catchError(() => { + // The HTTP interceptor shows the error; close the modal so a partially-loaded list can't be saved. + this.isLoadingMembers$.next(false); + this.closeModal(); + return EMPTY; + }) + ) + .subscribe((segment) => { + if (segment) { + this.applyEditFormValues(sourceList.listType, segment); + this.changeDetectorRef.markForCheck(); + } + this.isLoadingMembers$.next(false); + }) ); } } @@ -241,8 +257,7 @@ export class UpsertPrivateSegmentListModalComponent { this.setValidatorsBasedOnListType(listType); } - // True when the segment's member list wasn't loaded (counts-only) but a non-zero count - // indicates members exist, so the full list must be fetched before editing. + // True when members exist (count > 0) but weren't loaded, so they must be fetched before editing. private segmentMembersNeedFetch(listType: string, segment: Segment): boolean { if (!segment?.id || listType === LIST_OPTION_TYPE.SEGMENT) { return false; @@ -280,8 +295,11 @@ export class UpsertPrivateSegmentListModalComponent { listenForPrimaryButtonDisabled() { this.isPrimaryButtonDisabled$ = this.isUpsertLoading$.pipe( - combineLatestWith(this.isInitialFormValueChanged$), - map(([isLoading, isInitialFormValueChanged]) => isLoading || !isInitialFormValueChanged) + combineLatestWith(this.isInitialFormValueChanged$, this.isLoadingMembers$), + map( + ([isLoading, isInitialFormValueChanged, isLoadingMembers]) => + isLoading || isLoadingMembers || !isInitialFormValueChanged + ) ); this.subscriptions.add(this.isPrimaryButtonDisabled$.subscribe()); } diff --git a/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-details-participant-list-table/common-details-participant-list-table.component.ts b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-details-participant-list-table/common-details-participant-list-table.component.ts index f058d92e45..ec86904cb3 100644 --- a/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-details-participant-list-table/common-details-participant-list-table.component.ts +++ b/packages/frontend/projects/upgrade/src/app/shared-standalone-component-lib/components/common-details-participant-list-table/common-details-participant-list-table.component.ts @@ -92,8 +92,7 @@ export class CommonDetailsParticipantListTableComponent { const listType = rowData.listType; let count: number; - // Prefer the lightweight count returned by the details endpoint (member lists are not - // loaded there); fall back to the array length when the full lists are present. + // Prefer the count field (counts-only load); fall back to array length when full lists are present. if (listType?.toLowerCase() === this.memberTypes.INDIVIDUAL.toLowerCase()) { count = rowData.segment.individualForSegmentCount ?? rowData.segment.individualForSegment?.length ?? 0; } else { From 6682688ba3891b842d13310fcccb9e4791ec318a Mon Sep 17 00:00:00 2001 From: doswalt Date: Tue, 7 Jul 2026 08:59:35 -0400 Subject: [PATCH 15/16] add clarifying comment on import path --- packages/backend/src/api/services/FeatureFlagService.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/backend/src/api/services/FeatureFlagService.ts b/packages/backend/src/api/services/FeatureFlagService.ts index b4f3c3fd39..54501aa69f 100644 --- a/packages/backend/src/api/services/FeatureFlagService.ts +++ b/packages/backend/src/api/services/FeatureFlagService.ts @@ -1185,6 +1185,8 @@ export class FeatureFlagService { // The outer transaction has committed — recompute now (addList skipped it because it ran // inside the transaction) so the imported enabled lists are reflected in feature_flag_precomputed_segment. + // Unlike the interactive write paths (which fire-and-forget via withRecompute), import intentionally + // awaits so a successful import response means the precomputed rows are already consistent. await this.featureFlagPrecomputedSegmentService.recomputeForFlag(createdFlag.id, logger); } logger.info({ message: 'Imported feature flags', details: createdFlags }); @@ -1381,6 +1383,8 @@ export class FeatureFlagService { // The outer transaction has committed — recompute now (addList skipped it because it ran // inside the transaction) so the imported lists are reflected in feature_flag_precomputed_segment. + // Unlike the interactive write paths (which fire-and-forget via withRecompute), import intentionally + // awaits so a successful import response means the precomputed rows are already consistent. await this.featureFlagPrecomputedSegmentService.recomputeForFlag(featureFlagId, logger); logger.info({ message: 'Imported feature flags', details: createdLists }); From 0eb5c71f473a55cea134734f8470fcc4c4fe1cc8 Mon Sep 17 00:00:00 2001 From: doswalt Date: Tue, 7 Jul 2026 11:46:56 -0400 Subject: [PATCH 16/16] add fallback so a missing table will not crash the server on startup --- .../src/api/services/FeatureFlagService.ts | 15 ++++++++++++++- packages/backend/src/app.ts | 11 ++++++++++- .../unit/services/FeatureFlagService.test.ts | 18 ++++++++++++++++++ 3 files changed, 42 insertions(+), 2 deletions(-) diff --git a/packages/backend/src/api/services/FeatureFlagService.ts b/packages/backend/src/api/services/FeatureFlagService.ts index 407735928a..e7b310d3c6 100644 --- a/packages/backend/src/api/services/FeatureFlagService.ts +++ b/packages/backend/src/api/services/FeatureFlagService.ts @@ -3,6 +3,7 @@ import { FeatureFlag } from '../models/FeatureFlag'; import { Segment } from '../models/Segment'; import { FeatureFlagSegmentInclusion } from '../models/FeatureFlagSegmentInclusion'; import { FeatureFlagSegmentExclusion } from '../models/FeatureFlagSegmentExclusion'; +import { FeatureFlagPrecomputedSegment } from '../models/FeatureFlagPrecomputedSegment'; import { FeatureFlagRepository } from '../repositories/FeatureFlagRepository'; import { FeatureFlagExposureRepository } from '../repositories/FeatureFlagExposureRepository'; import { FeatureFlagSegmentInclusionRepository } from '../repositories/FeatureFlagSegmentInclusionRepository'; @@ -1029,7 +1030,19 @@ export class FeatureFlagService { logger: UpgradeLogger ): Promise[]> { const flagIds = featureFlags.map((f) => f.id); - const precomputedMap = await this.featureFlagPrecomputedSegmentService.getPrecomputedSets(flagIds); + // getPrecomputedSets can throw if the feature_flag_precomputed_segment table is unavailable + // (e.g. the migration hasn't been run yet). Treat that identically to every row being missing: + // swallow the error and fall through to on-the-fly segment resolution below rather than failing + // the whole assignment request. Rows self-heal on the next restart (backfill) or list mutation. + let precomputedMap: Map; + try { + precomputedMap = await this.featureFlagPrecomputedSegmentService.getPrecomputedSets(flagIds); + } catch (err) { + logger.error({ + message: `featureFlagLevelInclusionExclusion: failed to read feature_flag_precomputed_segment; falling back to on-the-fly resolution for all flags: ${err}`, + }); + precomputedMap = new Map(); + } // Build type-qualified group keys from the user's group map so they match the namespaced group // IDs stored in the precomputed arrays (individuals are matched bare against experimentUser.id). diff --git a/packages/backend/src/app.ts b/packages/backend/src/app.ts index 7e10ddd38b..2eb8f6f3bf 100644 --- a/packages/backend/src/app.ts +++ b/packages/backend/src/app.ts @@ -50,5 +50,14 @@ bootstrapMicroframework({ return createGlobalExcludeSegment(logger); }) .then(() => { - return backfillFeatureFlagPrecomputedSegments(logger); + // Best-effort: if the feature_flag_precomputed_segment table hasn't been migrated yet (or the + // backfill otherwise fails), log and continue instead of crashing startup with an unhandled + // rejection. The assignment read path falls back to on-the-fly segment resolution when a + // precomputed row — or the whole table — is unavailable, so the server stays fully functional; + // rows self-heal on a later restart (backfill) or list mutation (recompute). + return backfillFeatureFlagPrecomputedSegments(logger).catch((err) => { + logger.error({ + message: `feature_flag_precomputed_segment backfill failed at startup; continuing with on-the-fly fallback: ${err}`, + }); + }); }); diff --git a/packages/backend/test/unit/services/FeatureFlagService.test.ts b/packages/backend/test/unit/services/FeatureFlagService.test.ts index 1efe651c5d..10fc4d0ca1 100644 --- a/packages/backend/test/unit/services/FeatureFlagService.test.ts +++ b/packages/backend/test/unit/services/FeatureFlagService.test.ts @@ -936,6 +936,24 @@ describe('Feature Flag Service Testing', () => { expect(resolveSegmentsSpy).toHaveBeenCalledTimes(1); }); + + it('falls back to on-the-fly resolution (does not throw) when the precomputed table read fails', async () => { + const userDoc = { id: 'user123', group: {}, workingGroup: {} } as any; + const experimentAssignmentService = module.get(ExperimentAssignmentService); + const resolveSegmentsSpy = experimentAssignmentService.resolveSegmentsForEntities as jest.Mock; + const precomputed = module.get(FeatureFlagPrecomputedSegmentService); + + service.cacheService.wrap = jest.fn().mockResolvedValue([fastFlag]); + // Simulates the table not existing yet (e.g. migration not run): getPrecomputedSets rejects. + (precomputed.getPrecomputedSets as jest.Mock).mockRejectedValue( + new Error('relation "feature_flag_precomputed_segment" does not exist') + ); + resolveSegmentsSpy.mockResolvedValue([{}, {}]); + + // must resolve (not reject) and still route through the on-the-fly fallback + await expect(service.getKeys(userDoc, 'context1', logger)).resolves.toBeDefined(); + expect(resolveSegmentsSpy).toHaveBeenCalledTimes(1); + }); }); describe('precomputed recompute + seed triggers', () => {