Skip to content

docs(internal): update feature flags docs for cohort preloading in hypercache#52194

Open
inkeep[bot] wants to merge 2 commits intomasterfrom
docs-writer-ai-update-2026-03-24T22-36-55-830Z-rmeqsw
Open

docs(internal): update feature flags docs for cohort preloading in hypercache#52194
inkeep[bot] wants to merge 2 commits intomasterfrom
docs-writer-ai-update-2026-03-24T22-36-55-830Z-rmeqsw

Conversation

@inkeep
Copy link
Copy Markdown
Contributor

@inkeep inkeep bot commented Mar 24, 2026

Summary

Updates internal engineering documentation to reflect the cohort preloading changes from PR #52023.

Changes

docs/internal/feature-flags/hypercache-system.md

  • Updated cache payload structure to include the new optional cohorts array field
  • Added documentation for the cohorts field in the payload table
  • Added new "Cohort preloading" subsection explaining:
    • How _get_referenced_cohorts extracts cohort IDs from groups only (not super_groups or holdouts)
    • BFS loading via _load_cohorts_with_deps with depth limit 20, cycle-safe
    • Batch path that collects all cohort IDs across teams in a single BFS pass
    • Rust consumption via Option::take() with CohortCacheManager fallback
    • Rolling deploy safety (old Rust ignores unknown key; new Rust defaults to None)
  • Updated the "Models that trigger cache updates" list to note that Cohort changes now trigger service cache invalidation (with recalculation-only saves skipped)
  • Added cohort_changed_flags_cache signal handler entries to the signal handlers table

docs/internal/feature-flags/flag-evaluation-engine.md

  • Updated HyperCache JSON example to show the new cohorts field
  • Updated backwards compatibility section to reference cohorts alongside evaluation_metadata
  • Added introductory paragraph to "Cohort matching" section explaining the two data sources
  • Updated "Dynamic cohorts" step 1 to mention preloaded cohorts
  • Added new "Cohort data source" subsection with a table documenting preloaded vs fallback paths and the flags_cohort_source_total metric
  • Renamed "Cohort caching" to "Cohort caching (fallback)" with context that it's only used when preloaded cohorts are absent
  • Updated "Data fetching strategy" item 5 to reflect the new preloading behavior

Related


This PR was created by the Inkeep Content Writer agent, which is maintained by the Docs and Wizard team. Please reach out on Slack for help if needed.

inkeep bot added 2 commits March 24, 2026 22:39
Updates internal documentation for PR #52023:
- Add `cohorts` field to cache payload structure
- Add Cohort preloading subsection (BFS, batch, rolling deploy safety)
- Update Cohort cache invalidation entry (recalculation-only saves skipped)
- Add Cohort signal handlers to signal handlers table
…2023)

- Add cohorts field to HyperCache JSON example
- Update backwards compatibility section to cover cohorts
- Add Cohort data source subsection explaining preloaded vs fallback paths
- Rename Cohort caching to Cohort caching (fallback) with context
- Update data fetching strategy to reflect preloading behavior
- Document flags_cohort_source_total metric
@inkeep inkeep bot requested a review from haacked March 24, 2026 22:41
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 24, 2026

Prompt To Fix All With AI
This is a comment left during a code review.
Path: docs/internal/feature-flags/hypercache-system.md
Line: 144-148

Comment:
**Redundant table structure for `cohorts` field**

The section header is "The `cohorts` field:" and then the sole table row is also `cohorts`. This differs from the `evaluation_metadata` table just above it, where the rows document the *sub-fields* within `evaluation_metadata` (e.g., `dependency_stages`, `flags_with_missing_deps`, `transitive_deps`).

Since `cohorts` is a flat list rather than a nested object, there are no sub-fields to enumerate. The current table just re-states the field name, which may confuse readers who expect the table to describe the structure of the cohort dict objects within the list. Consider replacing the table with inline prose, or documenting the key fields of each cohort dict:

```markdown
The `cohorts` field is a `list[dict] | null` containing the serialized cohort definitions referenced by the team's flags, including transitive cohort-on-cohort dependencies. It is optional and absent in older cache entries. Each dict mirrors the `Cohort` model serialization used by the existing `CohortCacheManager`.
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: docs/internal/feature-flags/flag-evaluation-engine.md
Line: 320-327

Comment:
**"Backwards compatibility" heading removed**

The previous version had `#### Backwards compatibility` as a named subsection, which allowed direct linking (e.g., `#backwards-compatibility`). The updated text keeps the content but demotes it to an unlabelled paragraph. Given that the backwards compatibility guarantees are now more complex (covering both `evaluation_metadata` and `cohorts`), this section arguably deserves its heading more than before. Consider restoring it:

```suggestion
#### Backwards compatibility

The two paths are fully compatible via `#[serde(default)]` on `evaluation_metadata` and `cohorts`:
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "docs: update flag evaluation engine docs..." | Re-trigger Greptile

Comment on lines +144 to +148
The `cohorts` field:

| Field | Type | Description |
| --------- | ---------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `cohorts` | `list[dict]` \| `null` | Serialized cohort definitions referenced by the team's flags, including transitive cohort-on-cohort dependencies. Optional — absent in older cache entries. |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Redundant table structure for cohorts field

The section header is "The cohorts field:" and then the sole table row is also cohorts. This differs from the evaluation_metadata table just above it, where the rows document the sub-fields within evaluation_metadata (e.g., dependency_stages, flags_with_missing_deps, transitive_deps).

Since cohorts is a flat list rather than a nested object, there are no sub-fields to enumerate. The current table just re-states the field name, which may confuse readers who expect the table to describe the structure of the cohort dict objects within the list. Consider replacing the table with inline prose, or documenting the key fields of each cohort dict:

The `cohorts` field is a `list[dict] | null` containing the serialized cohort definitions referenced by the team's flags, including transitive cohort-on-cohort dependencies. It is optional and absent in older cache entries. Each dict mirrors the `Cohort` model serialization used by the existing `CohortCacheManager`.
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/internal/feature-flags/hypercache-system.md
Line: 144-148

Comment:
**Redundant table structure for `cohorts` field**

The section header is "The `cohorts` field:" and then the sole table row is also `cohorts`. This differs from the `evaluation_metadata` table just above it, where the rows document the *sub-fields* within `evaluation_metadata` (e.g., `dependency_stages`, `flags_with_missing_deps`, `transitive_deps`).

Since `cohorts` is a flat list rather than a nested object, there are no sub-fields to enumerate. The current table just re-states the field name, which may confuse readers who expect the table to describe the structure of the cohort dict objects within the list. Consider replacing the table with inline prose, or documenting the key fields of each cohort dict:

```markdown
The `cohorts` field is a `list[dict] | null` containing the serialized cohort definitions referenced by the team's flags, including transitive cohort-on-cohort dependencies. It is optional and absent in older cache entries. Each dict mirrors the `Cohort` model serialization used by the existing `CohortCacheManager`.
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines 320 to +327
4. Track missing dependencies (flags depending on non-existent flags)
5. Compute topological evaluation stages using Kahn's algorithm

#### Backwards compatibility
The two paths are fully compatible via `#[serde(default)]` on `evaluation_metadata` and `cohorts`:

The two paths are fully compatible via `#[serde(default)]` on `evaluation_metadata`:

- **Old Rust + new cache**: `evaluation_metadata` is an unknown field, ignored. Falls back to petgraph.
- **New Rust + old cache**: `evaluation_metadata` absent → `None` → falls back to petgraph.
- **New Rust + new cache**: `evaluation_metadata` present → fast pre-computed path.
- **Old Rust + new cache**: Unknown fields (`evaluation_metadata`, `cohorts`) are ignored. Falls back to petgraph and `CohortCacheManager`.
- **New Rust + old cache**: Fields absent → `None` → falls back to petgraph and `CohortCacheManager`.
- **New Rust + new cache**: `evaluation_metadata` present → fast pre-computed dependency path. `cohorts` present → skip `CohortCacheManager` PG query.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 "Backwards compatibility" heading removed

The previous version had #### Backwards compatibility as a named subsection, which allowed direct linking (e.g., #backwards-compatibility). The updated text keeps the content but demotes it to an unlabelled paragraph. Given that the backwards compatibility guarantees are now more complex (covering both evaluation_metadata and cohorts), this section arguably deserves its heading more than before. Consider restoring it:

Suggested change
4. Track missing dependencies (flags depending on non-existent flags)
5. Compute topological evaluation stages using Kahn's algorithm
#### Backwards compatibility
The two paths are fully compatible via `#[serde(default)]` on `evaluation_metadata` and `cohorts`:
The two paths are fully compatible via `#[serde(default)]` on `evaluation_metadata`:
- **Old Rust + new cache**: `evaluation_metadata` is an unknown field, ignored. Falls back to petgraph.
- **New Rust + old cache**: `evaluation_metadata` absent → `None` → falls back to petgraph.
- **New Rust + new cache**: `evaluation_metadata` present → fast pre-computed path.
- **Old Rust + new cache**: Unknown fields (`evaluation_metadata`, `cohorts`) are ignored. Falls back to petgraph and `CohortCacheManager`.
- **New Rust + old cache**: Fields absent → `None` → falls back to petgraph and `CohortCacheManager`.
- **New Rust + new cache**: `evaluation_metadata` present → fast pre-computed dependency path. `cohorts` present → skip `CohortCacheManager` PG query.
#### Backwards compatibility
The two paths are fully compatible via `#[serde(default)]` on `evaluation_metadata` and `cohorts`:
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/internal/feature-flags/flag-evaluation-engine.md
Line: 320-327

Comment:
**"Backwards compatibility" heading removed**

The previous version had `#### Backwards compatibility` as a named subsection, which allowed direct linking (e.g., `#backwards-compatibility`). The updated text keeps the content but demotes it to an unlabelled paragraph. Given that the backwards compatibility guarantees are now more complex (covering both `evaluation_metadata` and `cohorts`), this section arguably deserves its heading more than before. Consider restoring it:

```suggestion
#### Backwards compatibility

The two paths are fully compatible via `#[serde(default)]` on `evaluation_metadata` and `cohorts`:
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants