perf(server): shrink the field-metadata working set held live on the record query path - #24178
perf(server): shrink the field-metadata working set held live on the record query path#24178charlesBochet wants to merge 2 commits into
Conversation
The per-pod flatFieldMetadataMaps cap was 256 while ~470 distinct workspaces hit each pod at peak, so the cache constantly evicted and recomputed field-metadata entries. Each eviction costs a ~2s request-path recompute and mints a new live version that the fixed-rate packer cannot drain fast enough, growing the live heap and lengthening major GC pauses during the daily traffic peak. Sizing the cap above the active working set stops those evictions. The extra retained entries are mostly small packed blobs rather than live graphs, so the memory cost is modest.
|
🚀 Preview Environment Ready! Your preview environment is available at: https://joining-kitchen-without-light.trycloudflare.com This environment will automatically shut down after 5 hours. |
There was a problem hiding this comment.
Pull request overview
Raises the per-pod local cache cap for flatFieldMetadataMaps to reduce eviction-driven recomputation and associated GC pressure in the server workspace-cache subsystem.
Changes:
- Increased
flatFieldMetadataMapsper-key local cache entry cap from 256 to 512. - Added inline rationale/comments tying the cap to observed active workspace counts and recompute costs.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Greptile SummaryRaises the per-process
Confidence Score: 5/5The PR appears safe to merge, with the increased retention bounded by existing packing, expiration, and global cache controls. The change only adjusts one provider-specific cache cap, and the reviewed cache lifecycle packs cold versions, expires idle entries, and retains the existing global capacity bound. Important Files Changed
Reviews (1): Last reviewed commit: "perf(server): raise flatFieldMetadataMap..." | Re-trigger Greptile |
🟡 Standard review · 3 findings
High-level — Sound single-purpose perf refactor: a 💬 3 inline comments on the diff. Reviewed against the |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/twenty-server/src/engine/workspace-cache/services/workspace-cache.service.ts">
<violation number="1" location="packages/twenty-server/src/engine/workspace-cache/services/workspace-cache.service.ts:64">
P2: The new comment claims the extra retained `flatFieldMetadataMaps` entries are "mostly small packed blobs, not live graphs," but that is not accurate at the peak the cap is sized for. Packing only converts a version to a packed blob after MIN_IDLE_BEFORE_PACKING_MS (60s) of no reads, and a read (readVersion) flips it back to live. The ~470 workspaces this cap increase is meant to retain are precisely the actively-requested ones that churn through the cache at peak, so they stay live/unpacked. The sweep cap (sweepLocalCache) counts entries, not bytes, and does not distinguish live from packed, so raising the cap to 512 lets up to ~470 live graphs be retained simultaneously where the 256 cap previously forced evictions down to ~256 live. That can roughly double the flat-field live footprint (previously ~900MB-1GB per pod at the 256 pin) at a time when pods already sit near the 3.5GB heap ceiling, directly contradicting the "mostly small packed blobs" framing. The rollout note acknowledges the RSS risk, but the in-code justification understates it. Recommend either stating the tradeoff honestly or starting at the 384 middle ground noted in the PR.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const PACKING_PONDERATION_BUDGET = 64; | ||
| const MIN_IDLE_BEFORE_PACKING_MS = 60 * 1000; | ||
| // Per-provider entry caps, keyed by local cache key prefix (ORM graphs are ~5 MB each). | ||
| // flatFieldMetadataMaps is sized above the distinct active workspaces per pod (~470 at peak) so |
There was a problem hiding this comment.
P2: The new comment claims the extra retained flatFieldMetadataMaps entries are "mostly small packed blobs, not live graphs," but that is not accurate at the peak the cap is sized for. Packing only converts a version to a packed blob after MIN_IDLE_BEFORE_PACKING_MS (60s) of no reads, and a read (readVersion) flips it back to live. The ~470 workspaces this cap increase is meant to retain are precisely the actively-requested ones that churn through the cache at peak, so they stay live/unpacked. The sweep cap (sweepLocalCache) counts entries, not bytes, and does not distinguish live from packed, so raising the cap to 512 lets up to ~470 live graphs be retained simultaneously where the 256 cap previously forced evictions down to ~256 live. That can roughly double the flat-field live footprint (previously ~900MB-1GB per pod at the 256 pin) at a time when pods already sit near the 3.5GB heap ceiling, directly contradicting the "mostly small packed blobs" framing. The rollout note acknowledges the RSS risk, but the in-code justification understates it. Recommend either stating the tradeoff honestly or starting at the 384 middle ground noted in the PR.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/workspace-cache/services/workspace-cache.service.ts, line 64:
<comment>The new comment claims the extra retained `flatFieldMetadataMaps` entries are "mostly small packed blobs, not live graphs," but that is not accurate at the peak the cap is sized for. Packing only converts a version to a packed blob after MIN_IDLE_BEFORE_PACKING_MS (60s) of no reads, and a read (readVersion) flips it back to live. The ~470 workspaces this cap increase is meant to retain are precisely the actively-requested ones that churn through the cache at peak, so they stay live/unpacked. The sweep cap (sweepLocalCache) counts entries, not bytes, and does not distinguish live from packed, so raising the cap to 512 lets up to ~470 live graphs be retained simultaneously where the 256 cap previously forced evictions down to ~256 live. That can roughly double the flat-field live footprint (previously ~900MB-1GB per pod at the 256 pin) at a time when pods already sit near the 3.5GB heap ceiling, directly contradicting the "mostly small packed blobs" framing. The rollout note acknowledges the RSS risk, but the in-code justification understates it. Recommend either stating the tradeoff honestly or starting at the 384 middle ground noted in the PR.</comment>
<file context>
@@ -61,9 +61,12 @@ const PACKING_INTERVAL_MS = 500;
const PACKING_PONDERATION_BUDGET = 64;
const MIN_IDLE_BEFORE_PACKING_MS = 60 * 1000;
// Per-provider entry caps, keyed by local cache key prefix (ORM graphs are ~5 MB each).
+// flatFieldMetadataMaps is sized above the distinct active workspaces per pod (~470 at peak) so
+// the cap stops forcing evictions: an eviction there costs a ~2s request-path recompute, and the
+// extra retained entries are mostly small packed blobs, not live graphs.
</file context>
…ry path The ORM workspace context held the full flatFieldMetadataMaps live on every request. Each field carried the view/filter/sort/search/permission relation id arrays, a universal-identifier twin for each, and universalSettings - none of which the record query, RLS filtering or GraphQL/REST record resolvers read. They only need the scalar columns. Add a flatFieldMetadataMapsLite cache that projects the field map down to those scalar columns and repoint the ORM context to it. The full map is untouched and still serves metadata mutations, migrations, view resolution and the settings API; it is now read only on those infrequent paths, so it drops to the cold/packed tier and stops being traced live on every request. - LiteFlatFieldMetadata is an Omit of the dropped keys, so any hot-path read of a dropped property fails to compile. - Its cache is recomputed and invalidated as a derived sibling of flatFieldMetadataMaps from a single place in WorkspaceCacheService. - Shared field-access helpers and type guards are made generic over the field shape so both the full metadata and its lite projection flow through them without duplicating logic. - The lite recompute reads only field rows and unique indexes (2 queries) instead of the full builder's 9 joins.
charlesBochet
changed the title
There was a problem hiding this comment.
2 issues found across 35 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/twenty-server/src/engine/metadata-modules/flat-field-metadata/utils/is-flat-field-metadata-of-type.util.ts">
<violation number="1" location="packages/twenty-server/src/engine/metadata-modules/flat-field-metadata/utils/is-flat-field-metadata-of-type.util.ts:10">
P2: When a lite projection passes this guard, the predicate types it as `Field & FlatFieldMetadata<Type>`, reintroducing every omitted property. Code after the check can therefore compile a read such as `viewFieldIds` and receive `undefined`; narrow with the lite shape in the predicate result so full callers retain their fields through `Field` without reintroducing dropped keys.</violation>
</file>
<file name="packages/twenty-server/src/engine/metadata-modules/flat-field-metadata/utils/is-morph-or-relation-flat-field-metadata.util.ts">
<violation number="1" location="packages/twenty-server/src/engine/metadata-modules/flat-field-metadata/utils/is-morph-or-relation-flat-field-metadata.util.ts:16">
P2: When this guard narrows a lite field, `Field & FlatFieldMetadata<...>` adds back every property removed by the lite projection. Callers can then compile reads of omitted metadata and receive `undefined` at runtime; narrow with `LiteFlatFieldMetadata<MorphOrRelationFieldMetadataType>` instead.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // keep narrowing to the full field type as before. | ||
| export function isFlatFieldMetadataOfType< | ||
| Field extends FlatFieldMetadata<FieldMetadataType>, | ||
| Field extends LiteFlatFieldMetadata<FieldMetadataType>, |
There was a problem hiding this comment.
P2: When a lite projection passes this guard, the predicate types it as Field & FlatFieldMetadata<Type>, reintroducing every omitted property. Code after the check can therefore compile a read such as viewFieldIds and receive undefined; narrow with the lite shape in the predicate result so full callers retain their fields through Field without reintroducing dropped keys.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/metadata-modules/flat-field-metadata/utils/is-flat-field-metadata-of-type.util.ts, line 10:
<comment>When a lite projection passes this guard, the predicate types it as `Field & FlatFieldMetadata<Type>`, reintroducing every omitted property. Code after the check can therefore compile a read such as `viewFieldIds` and receive `undefined`; narrow with the lite shape in the predicate result so full callers retain their fields through `Field` without reintroducing dropped keys.</comment>
<file context>
@@ -1,9 +1,13 @@
+// keep narrowing to the full field type as before.
export function isFlatFieldMetadataOfType<
- Field extends FlatFieldMetadata<FieldMetadataType>,
+ Field extends LiteFlatFieldMetadata<FieldMetadataType>,
Type extends FieldMetadataType,
>(
</file context>
| Field extends LiteFlatFieldMetadata<FieldMetadataType>, | ||
| >( | ||
| flatFieldMetadata: Field, | ||
| ): flatFieldMetadata is Field & |
There was a problem hiding this comment.
P2: When this guard narrows a lite field, Field & FlatFieldMetadata<...> adds back every property removed by the lite projection. Callers can then compile reads of omitted metadata and receive undefined at runtime; narrow with LiteFlatFieldMetadata<MorphOrRelationFieldMetadataType> instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/metadata-modules/flat-field-metadata/utils/is-morph-or-relation-flat-field-metadata.util.ts, line 16:
<comment>When this guard narrows a lite field, `Field & FlatFieldMetadata<...>` adds back every property removed by the lite projection. Callers can then compile reads of omitted metadata and receive `undefined` at runtime; narrow with `LiteFlatFieldMetadata<MorphOrRelationFieldMetadataType>` instead.</comment>
<file context>
@@ -2,13 +2,19 @@ import {
+ Field extends LiteFlatFieldMetadata<FieldMetadataType>,
+>(
+ flatFieldMetadata: Field,
+): flatFieldMetadata is Field &
+ FlatFieldMetadata<MorphOrRelationFieldMetadataType> =>
isFlatFieldMetadataOfTypes(flatFieldMetadata, [
</file context>
🔍 Automated Pre-Review✅ No issues detected - This PR is ready for human review. Automated pre-review — human approval still required. |
![twenty-ci-bot-public[bot]](https://avatars.githubusercontent.com/u/119600397?s=60&v=4){kind=small}
| flatObjectMetadata, | ||
| flatFieldMetadataMaps, | ||
| ); | ||
| ) as unknown as FlatFieldMetadata[]; |
There was a problem hiding this comment.
🟡 Nit · Low-level · no as casts
New as unknown as FlatFieldMetadata[] cast widens the lite projection back to the full type
The function only returns string id maps so the generic T never reaches the output, and the loop's guard isFlatFieldMetadataOfType already narrows on the lite shape — the double cast (and its justifying comment) is avoidable. Type objectFields as LiteFlatFieldMetadata[] and let the guard narrow, rather than as unknown as FlatFieldMetadata[].
| }): LiteFlatFieldMetadata => { | ||
| const scalar = fromEntityToScalarEntity({ | ||
| metadataName: 'fieldMetadata', | ||
| entity: entity as EntityWithRegroupedOneToManyRelations< |
There was a problem hiding this comment.
🟡 Nit · Low-level · no as casts
Lite converter introduces two as casts the full converter avoids
entity as EntityWithRegroupedOneToManyRelations<...> and the return as LiteFlatFieldMetadata both defeat the compiler, and the sibling fromFieldMetadataEntityToFlatFieldMetadata needs neither. Align the parameter/return types (or build the object to satisfy the Omit) so the casts drop out, since these casts also erode the very compile-time guarantee the lite type is meant to give the hot path.
| import { type FlatObjectMetadata } from 'src/engine/metadata-modules/flat-object-metadata/types/flat-object-metadata.type'; | ||
|
|
||
| export const getFlatFieldsFromFlatObjectMetadata = ( | ||
| // Generic over the field shape so it preserves whatever the caller holds: the record query path |
There was a problem hiding this comment.
🟡 Nit · Low-level · comments
Added comments paraphrase the generic signatures they sit above
The <T extends LiteFlatFieldMetadata = FlatFieldMetadata> signature and one-line body already state what the comment says, so it fails the deletion test (same for the near-identical blocks on build-field-maps-from-flat-object-metadata and is-flat-field-metadata-of-type(s)). Drop the signature-restating comments and keep only the projection-rationale ones that encode an outside constraint.
Context
Investigating the prod-eu p99 latency spikes (10-14s at the daily traffic peak). The tail is an event-loop freeze from GC pressure: at peak, metadata version churn outruns the fixed-rate workspace-cache packer, so idle versions pile up as live object graphs instead of packed buffers, the live heap grows, and major-GC stop-the-world pauses lengthen.
Measured on prod-eu (PromQL):
flatFieldMetadataMapsentries per pod: pinned at the 256 cap, day and night — more workspaces contend for the cache than it can hold.Change
Raise the per-pod
flatFieldMetadataMapslocal cache cap from 256 → 512, above the distinct active workspaces per pod. This stops the cap from forcing evictions, each of which triggers a ~2s request-path recompute and mints a fresh live version that feeds the packer backlog.The extra retained entries are mostly small packed blobs rather than live object graphs, so the added memory is modest.
Rollout note
Pods run with
--max-old-space-size=3500. Watch per-pod RSS after deploy; if it creeps toward the heap ceiling, 384 is a safe middle ground.Follow-up (separate PR)
The larger root-cause fix is to shrink the field-metadata entry the record path holds live (a
flatFieldMetadataMapsLiteprojection dropping the view/permission relation arrays and their universal-identifier twins, ~60%+ smaller). That cascades the lite type through the wholetwenty-ormread subsystem and belongs in its own change.