Bound active-memory recall latency and jitter QMD startup#73667
Conversation
Greptile SummaryThis PR reduces the Active Memory recall deadline from 15 s to 3 s, removes the 30 s setup grace from the critical path, and adds per-agent jitter to stagger QMD startup. The jitter and timeout-reduction changes are sound, but the new
Confidence Score: 3/5Not safe to merge as-is — the hook envelope silently drops recall for any agent using a custom timeout above 3 s. A single P1 defect in index.ts means the hook timeout regression will affect any user who follows the docs guidance for full query mode (5,000 ms+), making recall silently fail. The latency reduction and jitter logic are otherwise correct, but the P1 pulls the score below the 4/5 ceiling. extensions/active-memory/index.ts line 2434 — beforePromptBuildTimeoutMs computation needs to account for the maximum configured timeoutMs. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/active-memory/index.ts
Line: 2434
Comment:
**Hook timeout uses `DEFAULT_TIMEOUT_MS` instead of the per-agent configured value**
`beforePromptBuildTimeoutMs` is now `DEFAULT_TIMEOUT_MS + setupGraceTimeoutMs + 1_000 = 4_000 ms`. The internal recall watchdog fires at `config.timeoutMs + setupGraceTimeoutMs` (line 2193), which uses the _per-agent_ `config.timeoutMs`, clamped up to 120,000 ms. Any agent configured with `timeoutMs > 3_000` (e.g., the docs update in this same PR recommends `5_000` or higher for `full` query mode) will have its hook cancelled at 4 s before the recall's own watchdog can fire, causing silent recall loss. The previous formula `120_000 + setupGraceTimeoutMs` was sized to always outlast the maximum allowed recall timeout.
To preserve the invariant, the hook envelope should wrap around the maximum possible recall duration, not the default:
```
// keep the hook envelope at the cap so per-agent timeoutMs up to 120 s still fits
const beforePromptBuildTimeoutMs = 120_000 + setupGraceTimeoutMs + 1_000;
```
If the goal is to reduce the _default_ latency without breaking custom high-timeout configurations, consider computing this per-session from the resolved `config.timeoutMs`, or at least use `Math.max(resolvedAgentTimeoutMs, DEFAULT_TIMEOUT_MS)` before adding the buffer.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/gateway/server-startup-memory.ts
Line: 51
Comment:
**`<= 0` guard is misleading for an array index**
`params.index` is the `.map()` callback index, which is always `>= 0`, so the `<= 0` condition is functionally identical to `=== 0` but implies negative indices are possible. Using strict equality makes the intent clearer.
```suggestion
if (params.index === 0 || params.total <= 1) {
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(active-memory): bound recall latency..." | Re-trigger Greptile |
| }); | ||
|
|
||
| const beforePromptBuildTimeoutMs = 120_000 + setupGraceTimeoutMs; | ||
| const beforePromptBuildTimeoutMs = DEFAULT_TIMEOUT_MS + setupGraceTimeoutMs + 1_000; |
There was a problem hiding this comment.
DEFAULT_TIMEOUT_MS instead of the per-agent configured value
beforePromptBuildTimeoutMs is now DEFAULT_TIMEOUT_MS + setupGraceTimeoutMs + 1_000 = 4_000 ms. The internal recall watchdog fires at config.timeoutMs + setupGraceTimeoutMs (line 2193), which uses the per-agent config.timeoutMs, clamped up to 120,000 ms. Any agent configured with timeoutMs > 3_000 (e.g., the docs update in this same PR recommends 5_000 or higher for full query mode) will have its hook cancelled at 4 s before the recall's own watchdog can fire, causing silent recall loss. The previous formula 120_000 + setupGraceTimeoutMs was sized to always outlast the maximum allowed recall timeout.
To preserve the invariant, the hook envelope should wrap around the maximum possible recall duration, not the default:
// keep the hook envelope at the cap so per-agent timeoutMs up to 120 s still fits
const beforePromptBuildTimeoutMs = 120_000 + setupGraceTimeoutMs + 1_000;
If the goal is to reduce the default latency without breaking custom high-timeout configurations, consider computing this per-session from the resolved config.timeoutMs, or at least use Math.max(resolvedAgentTimeoutMs, DEFAULT_TIMEOUT_MS) before adding the buffer.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/active-memory/index.ts
Line: 2434
Comment:
**Hook timeout uses `DEFAULT_TIMEOUT_MS` instead of the per-agent configured value**
`beforePromptBuildTimeoutMs` is now `DEFAULT_TIMEOUT_MS + setupGraceTimeoutMs + 1_000 = 4_000 ms`. The internal recall watchdog fires at `config.timeoutMs + setupGraceTimeoutMs` (line 2193), which uses the _per-agent_ `config.timeoutMs`, clamped up to 120,000 ms. Any agent configured with `timeoutMs > 3_000` (e.g., the docs update in this same PR recommends `5_000` or higher for `full` query mode) will have its hook cancelled at 4 s before the recall's own watchdog can fire, causing silent recall loss. The previous formula `120_000 + setupGraceTimeoutMs` was sized to always outlast the maximum allowed recall timeout.
To preserve the invariant, the hook envelope should wrap around the maximum possible recall duration, not the default:
```
// keep the hook envelope at the cap so per-agent timeoutMs up to 120 s still fits
const beforePromptBuildTimeoutMs = 120_000 + setupGraceTimeoutMs + 1_000;
```
If the goal is to reduce the _default_ latency without breaking custom high-timeout configurations, consider computing this per-session from the resolved `config.timeoutMs`, or at least use `Math.max(resolvedAgentTimeoutMs, DEFAULT_TIMEOUT_MS)` before adding the buffer.
How can I resolve this? If you propose a fix, please make it concise.| } | ||
|
|
||
| function resolveStartupJitterMs(params: { index: number; total: number }): number { | ||
| if (params.index <= 0 || params.total <= 1) { |
There was a problem hiding this comment.
<= 0 guard is misleading for an array index
params.index is the .map() callback index, which is always >= 0, so the <= 0 condition is functionally identical to === 0 but implies negative indices are possible. Using strict equality makes the intent clearer.
| if (params.index <= 0 || params.total <= 1) { | |
| if (params.index === 0 || params.total <= 1) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/server-startup-memory.ts
Line: 51
Comment:
**`<= 0` guard is misleading for an array index**
`params.index` is the `.map()` callback index, which is always `>= 0`, so the `<= 0` condition is functionally identical to `=== 0` but implies negative indices are possible. Using strict equality makes the intent clearer.
```suggestion
if (params.index === 0 || params.total <= 1) {
```
How can I resolve this? If you propose a fix, please make it concise.|
Codex review: needs real behavior proof before merge. Summary Reproducibility: yes. for the PR regression by source inspection: configure Active Memory on the branch with timeoutMs above 3000 ms and the fixed 4000 ms hook timeout can fire before the configured recall budget. I did not run a live gateway canary in this read-only review. Real behavior proof Next step before merge Security Review findings
Review detailsBest possible solution: Rework or replace the branch so hook budgets preserve configured recall timeouts, build on current-main setup-grace and QMD lazy-start semantics, and leave broader nonblocking/default-policy work on the canonical reliability tracker. Do we have a high-confidence way to reproduce the issue? Yes, for the PR regression by source inspection: configure Active Memory on the branch with timeoutMs above 3000 ms and the fixed 4000 ms hook timeout can fire before the configured recall budget. I did not run a live gateway canary in this read-only review. Is this the best way to solve the issue? No, not as proposed. The reliability direction is plausible, but the hook envelope must be sized from the resolved configured timeout or cap, and the QMD/default-latency policy should be reconciled with current main before merge. Full review comments:
Overall correctness: patch is incorrect What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 48b4e5b3614f. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
This assigned pull request has been automatically marked as stale after being open for 27 days. |
|
Closing due to inactivity. |
Summary
This patch applies the active-memory reliability approach proposed by @kinthaiofficial in #72015:
before_prompt_buildhook deadline-bounded so memory recall degrades instead of blocking normal repliesThe existing QMD search manager cache is already keyed per agent, so this keeps the change small and focuses on the blocking failure mode plus startup herd behavior.
Tests
pnpm exec vitest run extensions/active-memory/index.test.ts src/gateway/server-startup-memory.test.tspnpm tsgo:extensions:testpnpm tsgo:core:testFixes #72015