Repository navigation
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.
Devin Review found 4 potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| async start(options: ChildStartOptions): Promise<void> { | ||
| this.send({ type: "start", options }); | ||
| await this.ready; | ||
| if (this.fatal) throw this.fatal; | ||
| if (this.port === 0) | ||
| throw new Error("benchmark child did not report a port"); |
There was a problem hiding this comment.
🔴 Restart races live benchmark process
When restarting, stop() returns immediately after signaling the child, so the next process can open its still-active database. Cleanup can also remove the database before the child exits.
Learn more
The parent creates two serving processes against the same fixture database and expects a clean process boundary. stop() sends a signal and disconnects IPC without awaiting the child’s exit; runMixedLoadBenchmark immediately starts the second child, and its cleanup removes the fixture after another nonblocking stop. The child has a shutdown handler for an IPC stop message, but the parent never sends it.
Example: Process A receives SIGTERM while finishing a database operation. Process B starts on the same root before A exits; process A can finish writing after B's initial persistence snapshot.
Recommended fix: Send the IPC stop message to permit orderly stop, wait for the child's exit event with a bounded deadline and SIGKILL fallback, and remove the root only after exit.
Was this helpful? React with 👍 or 👎 to provide feedback.
Coverage Results 📊✅ Patch coverage is 81.42% (263 of 323 changed executable lines covered; target 80%). Changed files with executable lines (5)
Coverage diff@@ Coverage Diff @@
## main #1896 +/-##
==========================================
- Coverage 84.67% 84.65% -0.02%
==========================================
Files 306 309 +3
Tracked lines 48411 48734 +323
Branches 39354 39573 +219
==========================================
+ Hits 40990 41253 +263
- Misses 7421 7481 +60
- Partials 4212 4254 +42Generated by Coverage Action |
There was a problem hiding this comment.
Devin Review found 4 new potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| const allStats = [...heldStats, ...finalStats]; | ||
| const upstreamMeasurements = allStats.flatMap( | ||
| (stats) => stats.upstreamMeasurements, | ||
| ); |
There was a problem hiding this comment.
🟡 Upstream measurements double-count earlier requests
When held snapshots exist, upstreamMeasurements concatenates them with cumulative final snapshots. Requests captured before each held snapshot appear twice in the report.
Learn more
Each statsReader returns every upstream measurement recorded since child startup. heldStats and finalStats contain overlapping snapshots for the same process, but the report concatenates both arrays. The checked-in reference report consequently contains duplicated upstream rows, obscuring actual request counts and distorting any downstream aggregation.
Example: One base request and one append request produce two measurements in the final snapshot; a held snapshot already contains both. Concatenation reports four upstream requests for that process rather than two.
Recommended fix: Build upstream measurements from one final snapshot per process, or store only the delta between successive snapshots.
Was this helpful? React with 👍 or 👎 to provide feedback.
| exactUpstreamHashes: upstreamMeasurements.every( | ||
| (measurement) => | ||
| /^[0-9a-f]{64}$/.test(measurement.bodyHash) && | ||
| /^[0-9a-f]{64}$/.test(measurement.toolSequenceHash) && | ||
| /^[0-9a-f]{64}$/.test(measurement.provenanceHash), | ||
| ), |
There was a problem hiding this comment.
🟡 Upstream hash gate misses changed payloads
If the gateway alters tool or provenance content, exactUpstreamHashes still passes because it checks only SHA-256 formatting. The report marks mismatched upstream requests as exact.
Learn more
The upstream server computes bodyHash, toolSequenceHash, and provenanceHash from the actual forwarded input in upstreamMeasurements. The gate validates only their 64-character shape. Every SHA-256 result has that shape regardless of whether the forwarded payload matches the generated workload, so the invariant does not detect a content regression.
Example: If translation drops every reasoning item, provenanceHash changes but remains 64 hexadecimal characters; exactUpstreamHashes stays true.
Recommended fix: Independently derive expected hashes from the generated workload or an explicitly defined allowed transformation, then compare forwarded values against them; include count and order expectations for each sampled request.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (!timing || !observer) return; | ||
| timings.set(timing.requestId, { ...timing, decodedAt: performance.now() }); |
There was a problem hiding this comment.
🟡 Early responses retain benchmark timing records
For a benchmark-tagged request handled before normal upstream forwarding, timings retains its entry. Repeated early responses grow the map and can leave stale measurements for reused IDs.
Learn more
The Responses route calls finishBenchmarkDecode before sending every parsed request to the pipeline. Only the normal conversation's upstream path calls clearBenchmarkTiming; handleRequestInner also has nonthrowing early returns, and its slash, compaction, and meta branches return without clearing timings. Entries persist as long as the process keeps the observer active.
Example: A benchmark-tagged Responses request rejected for conflicting auth headers stores its ID in timings and receives HTTP 400. Sending thousands of such requests accumulates thousands of entries.
Recommended fix: Give each decoded request a guaranteed cleanup path when request handling settles, while preserving the upstream timing event; also clear any remaining entries when disabling the observer.
Was this helpful? React with 👍 or 👎 to provide feedback.
| for (;;) { | ||
| await new Promise<void>((resolve) => setImmediate(resolve)); | ||
| if (embeddingState.running === 0) break; | ||
| for (const worker of controlledEmbeddingWorkers) | ||
| worker.releaseAll(core.config().search.embeddings.dimensions); | ||
| } | ||
| if (activeDrain) { | ||
| try { | ||
| await activeDrain; |
There was a problem hiding this comment.
🟡 Embedding release misses late follow-up work
If the drain starts another embed after running reaches zero, releaseScenario stops releasing workers before awaiting activeDrain. The barrier can stall on an unreleased embed.
Learn more
The release loop looks only at the current embeddingState.running count, but the comment explains that a completed temporal write can start follow-up embedding work as the original drain settles. The code checks for zero before awaiting the outstanding activeDrain, so a later embed can be added after the loop ends. That embed stays held by ControlledEmbeddingWorker until its 30-second timeout.
Example: The first embed completes; a setImmediate sees zero running embeds and exits the loop. The drain resumes and starts a second controlled embed. The subsequent await activeDrain waits on that embed without any remaining release loop.
Recommended fix: Continue releasing pending controlled embeds until the drain has settled and the running count remains zero after a scheduling turn; impose a bounded overall release deadline.
Was this helpful? React with 👍 or 👎 to provide feedback.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| return protocol.startsWith(SOURCE_CHECKPOINT_PROTOCOL_PREFIX); | ||
| } | ||
| export const SOURCE_WINDOW_MAX_MESSAGES = 2048; | ||
| export const SOURCE_WINDOW_MAX_MESSAGES = 8192; |
There was a problem hiding this comment.
Bug: Increasing SOURCE_WINDOW_MAX_MESSAGES without updating PREFIX_COUNTS causes checkpoints to become ineffective for large conversations, forcing a fallback to full-source processing.
Severity: HIGH
Suggested Fix
Increase the PREFIX_COUNTS constant to match or exceed the new SOURCE_WINDOW_MAX_MESSAGES value of 8192. This ensures the prefixTokens array is correctly populated for offsets up to the new maximum window size, preventing the fallback to full-source processing.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/gateway/src/source-checkpoint.ts#L33
Potential issue: Increasing `SOURCE_WINDOW_MAX_MESSAGES` to 8192 without a corresponding
increase in `PREFIX_COUNTS` (which remains at 4096) causes checkpointing to fail for
large conversations. When a conversation is pruned and the cumulative message offset
exceeds 4096, the `finishWindow` method no longer extends the `prefixTokens` array. On
subsequent requests, code attempting to access `prefixTokens` at an offset greater than
4096 will find an `undefined` value. This triggers a `FullSourceRequired("calibration")`
exception, forcing a fallback to slower, full-source processing and negating the
performance benefit of using checkpoints for the exact workloads this change is intended
to improve.
Did we get this right? 👍 / 👎 to inform future reviews.
This branch has not been deployed
1 participant
Summary
Validation
pnpm exec vitest run packages/gateway/test/mixed-load-benchmark.test.ts packages/gateway/test/source-checkpoint.test.ts --printConsoleTracepnpm --filter @loreai/gateway run typecheckpnpm exec oxlint --type-aware packages/gateway/benchmark/mixed-load packages/gateway/src/benchmark-timing.ts packages/gateway/src/source-checkpoint.ts packages/gateway/test/mixed-load-benchmark.test.tspnpm run benchmark:mixed-load -- --quick --output /tmp/opencode/mixed-load-final.jsonpnpm run format:checkFull-repository lint still reports pre-existing warnings outside this change; the scoped lint command passes.
Closes #1733