perf(request): optimize hot-path transformations and add runtime benchmark harness#17
perf(request): optimize hot-path transformations and add runtime benchmark harness#17ndycode wants to merge 3 commits into
Conversation
Avoid extra array passes in filterInput and clone only tool schema trees during cleanupToolDefinitions, preserving behavior while reducing hot-path work. Co-authored-by: Codex <noreply@openai.com>
Add a reusable benchmark script and npm commands to measure request-path hotspots (filterInput, cleanupToolDefinitions, and account selection) for before/after performance tracking. Co-authored-by: Codex <noreply@openai.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Prevent sparse-array holes from causing runtime errors in the single-pass filterInput loop and add regression coverage for sparse entries. Co-authored-by: Codex <noreply@openai.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Fixed items
Deferred items
Blocked items
Validation evidencepm run clean:repo:check - passpm run audit:ci - passpm run typecheck - passpm run lint - passpm run build - passpm test - pass (92 files, 2113 tests)
|
Completes partial fixes flagged in the third review, bumps to v2.2.1. Real bugs (prior fixes were incomplete): - request/fetch-helpers: isUnsupportedCodexModelForChatGpt (the handleErrorResponse path) now also matches NORMALIZED_UNSUPPORTED_MODEL_PATTERN, so a 400 'model not currently available for this chatgpt account' gets the entitlement rewrite, not generic error guidance (#13) - forecast: quota-exhausted accounts are classified 'delayed' (not 'unavailable'), so they slipped the recommendation filter; added an explicit exhausted flag and excluded it, returning null when the whole pool is exhausted (#10) - storage/record-utils: clampIndex guards NaN -> 0 (Math.trunc(NaN) propagated) (#16) - local-client-tokens: debounce lastUsedAt persistence (60s threshold) so the bearer-verify hot path stops writing to disk every request; +chmod 0o700 re-assert on the token-store dir (#12, #11) - mcodex: relay SIGTERM/SIGINT to the spawned child so it isn't orphaned (#15) Test quality: - local-client-tokens: parameterized rename-retry test over ENOTEMPTY/EAGAIN/EACCES (#18) - storage-flagged: clear the H4 deadlock-guard timer on the happy path (#17) - storage: removeWithRetry for suite cleanup (#19) Release: - bump package.json + .codex-plugin/plugin.json to 2.2.1 - add docs/releases/v2.2.1.md; point docs portal + README at it Known follow-ups (documented, deferred — need design, not rushed into a patch): config env-path save is a single-process CAS not a true cross-process lock (#8/#9); verifyLocalClientBearerToken read stays serialized but a fuller lease is future work; runtime proxy routingMutex='enabled' still has a select/commit cursor race (#14) requiring an async refactor of chooseAccount across its call sites. Full suite: 4337 passed, 3 skipped, 0 failed; typecheck + lint clean.
Real bug introduced by the round-3 config lock (#18): - the cross-process config lock fixed expiresAt once and never renewed, and released by unconditionally unlinking the lockfile. A save running longer than CONFIG_LOCK_TTL_MS could be deemed stale and stolen by another process, after which the original holder would delete the NEW owner's lock and reopen concurrent saves. Added a per-acquisition owner token (randomUUID) to the lock payload and a releaseConfigLockIfOwner() that compares-before-unlink, so a holder never deletes a lock it no longer owns. Regressions: stale-foreign-lock takeover (cleans only its own lock) + live-foreign-lock respected (times out, foreign lock untouched, no partial apply). Nits: - mcodex-launcher test: dropped the explicit vitest globals import (#17). - runtime-rotation-proxy: replaced the stale 'KNOWN GAP (L4)' comment on chooseAccount with accurate docs — the race is closed via the reentrant withRoutingMutex on the hot path (#19). Full suite: 4343 passed, 3 skipped, 0 failed; typecheck + lint clean.
Summary
pm run bench:runtime-path
pm run bench:runtime-path:quick
Performance Evidence
Baseline and after measurements (same local micro-benchmark shape, Node v25.6.1):
Current reusable harness output (
ode scripts/benchmark-runtime-path.mjs --iterations=60):
Validation
pm run typecheck ✅
pm run build ✅
Targeted regression suites ✅
px vitest run test/request-transformer.test.ts test/property/transformer.property.test.ts
pm test⚠️ 1 known pre-existing failure on main:
Risk/Compatibility
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
reduced request-path allocation overhead by optimizing
filterInput(single-pass iteration) andcleanupToolDefinitions(selective cloning). added sparse array guard to prevent crashes on malformed inputs with regression test coverage.filterInputnow uses for-loop instead of filter/map chains, guards against sparse/null entriescleanupToolDefinitionsshallow-spreads tool objects, only deep-clones mutated parameters schemawindows filesystem/token safety: not applicable - no file i/o or credential handling in hot-path changes. benchmark script only writes json output with standard mkdir/writeFile, no cleanup operations requiring retry logic.
vitest coverage: existing tool-utils tests already cover undefined property cleanup (lines 272-309), property-based tests verify filterInput behavior parity.
Confidence Score: 5/5
Important Files Changed
filterInputfrom filter/map chains to single-pass for loop, added sparse array guard to prevent crashes on malformed inputscleanupToolDefinitionsto use shallow spread + selective deep clone instead of full deep clone, added undefined property cleanup to maintain behavior paritybench:runtime-pathandbench:runtime-path:quickscripts for runtime performance testingLast reviewed commit: 9cd71c4