fix: unflatten crash on large numeric keys and fair-queue age ranking - #4496
fix: unflatten crash on large numeric keys and fair-queue age ranking#4496eeshsaxena wants to merge 2 commits into
Conversation
…tening An object whose keys are all large numbers (for example millisecond timestamps) was being turned into an array, so Array(maxIndex + 1) threw "Invalid array length" (or allocated a huge array). Only rebuild an array when the keys are real array indices (< 2^32 - 1), otherwise return the object as-is.
selectTopTenantQueues weighted tenants by the average of their queue scores, but a score is the oldest-message timestamp (lower means older). That ranked newer tenants higher and, since timestamps are all close in magnitude, made the weights nearly identical. Weight by age (now - score) so the tenants waiting the longest are prioritized.
🦋 Changeset detectedLatest commit: 96b072b The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Hi @eeshsaxena, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // Only rebuild an array when every key is a real array index (< 2^32 - 1). A | ||
| // larger numeric key, like a millisecond timestamp used as an object key, is | ||
| // not an array index, so keep the object form instead of throwing "Invalid | ||
| // array length" or allocating a huge array. | ||
| if (maxIndex < MAX_ARRAY_INDEX) { | ||
| const arrayResult = Array(maxIndex + 1); | ||
| for (const key of topLevelKeys) { | ||
| arrayResult[parseInt(key)] = result[key]; | ||
| } | ||
| return arrayResult as any; | ||
| } |
There was a problem hiding this comment.
🟡 Objects keyed by second-precision timestamps still expand into billion-slot lists
The cutoff for treating numeric keys as list positions is set at roughly 4.29 billion (maxIndex < MAX_ARRAY_INDEX at packages/core/src/v3/utils/flattenAttributes.ts:356), so data keyed by second-precision timestamps (about 1.7 billion) is still turned into a list with billions of empty slots, so displaying or saving that data can exhaust memory.
Impact: A task payload or output that uses second-based timestamps (or any other large-but-under-4-billion number) as its keys can make the server run out of memory when that data is written or rendered.
Why the 2^32 threshold does not cover the realistic key range
unflattenAttributes only avoids the array rebuild when the largest numeric key is >= 2^32-1. A unix timestamp in seconds (e.g. 1700000000) is below that, so Array(1700000001) is still allocated and every key is written into it. Downstream this value is serialized, e.g. JSON.stringify(unflattenAttributes(...)) in apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts:697, which would materialize ~1.7 billion null, entries and blow up memory. A density/size-based heuristic (e.g. only rebuild the array when maxIndex + 1 is within a small multiple of the number of keys, or below a modest constant like 100k) would cover both the millisecond and second timestamp cases.
Prompt for agents
In packages/core/src/v3/utils/flattenAttributes.ts, unflattenAttributes converts a result whose top-level keys are all numeric into an array. The new guard only skips the conversion when the largest key is >= 2^32-1, which prevents the RangeError for millisecond timestamps but still allows huge sparse arrays for smaller-but-large keys (e.g. second-precision unix timestamps around 1.7e9). Those arrays are later serialized (see apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts:697) and can exhaust memory. Consider a density-based check instead: only rebuild the array when the resulting length is reasonable relative to the number of keys (e.g. maxIndex + 1 <= keys.length * someSmallFactor, or below a modest absolute cap), otherwise keep the object form. Update the tests accordingly.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Calculate average age per tenant. A queue's score is its oldest message | ||
| // timestamp, so age is now - score. Older queues have a higher age and | ||
| // should get more weight when we pick the top tenants. | ||
| const tenantAges = Array.from(queuesByTenant.entries()).map(([tenantId, tQueues]) => { | ||
| const avgAge = tQueues.reduce((sum, q) => sum + q.score, 0) / tQueues.length; | ||
| const avgAge = tQueues.reduce((sum, q) => sum + (now - q.score), 0) / tQueues.length; |
There was a problem hiding this comment.
🔍 Age-based weighting can drive freshly-enqueued tenants to weight 0
With avgAge = now - score, a tenant whose queues were just written has avgAge ≈ 0 and therefore weight ≈ 0. In the selection loop (packages/redis-worker/src/fair-queue/schedulers/weighted.ts:266-285) the inner while (random > 0 ...) never stops on a zero-weight entry, so such tenants are effectively only reachable via the index = Math.max(0, index - 1) fallback or once they are the only ones left. Previously (weighting by raw timestamp) all weights were near-identical, so selection was near-uniform. This is the intended direction of the fix, but it does mean the newest tenants can be nearly starved while maximumTenantCount is smaller than the tenant population — worth confirming that's acceptable, since #getQueuesFromShard already caps results at masterQueueLimit in score (oldest-first) order.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
WalkthroughThe change updates fair queue tenant selection to calculate queue age from the current timestamp and queue score. Older queues receive greater selection weight. It also updates attribute unflattening to preserve empty results and objects with numeric keys at or above the valid array index limit. Tests cover large numeric keys at the root and within nested objects. Two patch changesets document these fixes. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Two small, independent fixes. I kept them in one PR since both are low risk patches, but happy to split them if you'd rather.
✅ Checklist
Testing
I couldn't run the full monorepo suite locally, so I validated both changes with isolated reproductions and added a regression test for the first one. CI should cover the rest.
1. unflattenAttributes crash
An object whose keys are all large numbers (for example millisecond timestamps) was being turned into an array, so
Array(maxIndex + 1)threwRangeError: Invalid array length:This is reachable from normal task data, since run input and output get flattened for telemetry and unflattened again for display. After the fix those values come back as an object, and normal arrays are unchanged. Added two tests in
packages/core/test/flattenAttributes.test.ts.2. Fair queue tenant ranking
WeightedSchedulerselectTopTenantQueues(used whenmaximumTenantCountis set) weighted tenants by the average of their queue scores. A score is the queue's oldest message timestamp, where a lower score means older and higher priority, so ranking by score favored the newer tenants. And because the timestamps are all close in magnitude, the weights came out nearly identical, which made the selection close to uniform random. The fix weights by age (now - score), which is what the other methods in the same file already do.Changelog
@trigger.dev/core: fix a crash when unflattening attributes that contain an object with large numeric keys.@trigger.dev/redis-worker: fair queue now picks the tenants waiting the longest when a maximum tenant count is set.Changesets are included for both.