feat(summarize): report failures to Sentry + configurable retries#1051
Open
rexplx wants to merge 16 commits into
Open
feat(summarize): report failures to Sentry + configurable retries#1051rexplx wants to merge 16 commits into
rexplx wants to merge 16 commits into
Conversation
mem::summarize failures (empty_provider_response, parse_failed, validation_failed, and thrown provider errors) were only written to the structured logger, so they never surfaced in Sentry even when the SDK was available. Wire all four failure sites into Sentry and make the top-level produce-and-parse retry configurable (default 3, was a hard 2). - New src/observability/sentry.ts: hard no-op unless SENTRY_DSN is set, so builds/tests/deploys without a DSN are unaffected (@sentry/node added). - initSentry() called once at worker startup in src/index.ts. - SUMMARIZE_MAX_ATTEMPTS env (default 3) controls the retry loop; a third attempt recovers a meaningful fraction of empty/parse failures. Motivated by a lifetime ~20.7% summarize failure rate that was invisible in Sentry because the engine imported no @sentry SDK.
summarizeChunkWithRetry used a hard 2 attempts. Bump to a configurable default of 3 via SUMMARIZE_CHUNK_MAX_ATTEMPTS, kept as a separate knob from SUMMARIZE_MAX_ATTEMPTS because chunk retries multiply across every chunk and concurrency slot — large sessions under provider throttling can amplify load, so ops can tune the chunk knob down independently of the cheap top-level retry. The skip-ratio bailout (MAX_SKIP_RATIO) still guards against half-blind merges.
|
@rexplx is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds configurable chunk and top-level summarization retries with backoff and caching, optional Sentry lifecycle integration, safe provider error handling, and Fly deployment builds from a locally packed repository tarball. ChangesSummarization and observability
Provider error safety
Fly local package build
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Worker
participant Summarizer
participant Provider
participant Sentry
Worker->>Summarizer: start configured retry loop
Summarizer->>Provider: request chunk or final summary
Provider-->>Summarizer: response or safe error
Summarizer->>Summarizer: back off, reuse cached chunks, or classify failure
Summarizer->>Sentry: capture structured failure or exception
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/observability/sentry.ts (1)
15-35: 🩺 Stability & Availability | 🔵 TrivialConsider exporting a
flushSentry()helper for graceful shutdown.The shutdown handler in
src/index.tscallsprocess.exit(0)without flushing Sentry's event queue. Events captured just before shutdown (e.g., acaptureSummarizeExceptionduring the final invocation) could be lost. Export aflushSentry(timeout)wrapper from this module and call it in the shutdown path beforeprocess.exit.📋 Suggested addition
/** Flush pending Sentry events. No-op unless Sentry is enabled. */ export async function flushSentry(timeoutMs = 2000): Promise<void> { if (!enabled) return; try { await Sentry.flush(timeoutMs); } catch { /* swallow */ } }Then in the shutdown handler in
src/index.ts:await sdk.shutdown(); + await flushSentry(); clearWorkerPidfile(); process.exit(0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/observability/sentry.ts` around lines 15 - 35, Export a flushSentry(timeoutMs) helper alongside initSentry that returns immediately when Sentry is disabled, awaits Sentry.flush with the provided timeout, and swallows flush errors. Update the shutdown handler in the process lifecycle code to await flushSentry before calling process.exit(0).src/functions/summarize.ts (1)
309-319: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale comment references "2-attempt loop".
The comment on lines 309-313 says "the same 2-attempt loop" but the code now uses
getMaxAttempts()(default 3, configurable viaSUMMARIZE_MAX_ATTEMPTS). This could mislead developers into thinking the retry count is hardcoded.📝 Update comment to reflect configurable retries
- // `#783`: chunk-level produceSummaryXml retries internally, but - // the final merge used to parse once and bail. Wrap the - // produce-and-parse pair in the same 2-attempt loop so a - // markdown-wrapped or otherwise wrapped response gets a - // second roll-of-the-dice instead of dropping the summary. + // `#783`: chunk-level produceSummaryXml retries internally, but + // the final merge used to parse once and bail. Wrap the + // produce-and-parse pair in a configurable retry loop + // (SUMMARIZE_MAX_ATTEMPTS, default 3) so a markdown-wrapped + // or otherwise wrapped response gets another roll-of-the-dice + // instead of dropping the summary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/functions/summarize.ts` around lines 309 - 319, The comment above the retry loop in the summarize flow references a fixed “2-attempt loop,” but the loop uses configurable getMaxAttempts(). Update that comment to describe the shared/configurable retry behavior without stating a hardcoded attempt count; leave the implementation unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/functions/summarize.ts`:
- Around line 454-459: Update the context object passed to
captureSummarizeException in the summarize catch path to include the enclosing
mode and chunks values, alongside sessionId, provider, observationCount, and
latencyMs.
In `@src/observability/sentry.ts`:
- Line 28: In the Sentry initialization flow, extract the resolved environment
value used by the Sentry configuration into a variable and reuse that variable
in the “Sentry initialized” logger.info call. Ensure the log matches the
existing NODE_ENV, production fallback, and FLY_APP_NAME resolution exactly.
---
Nitpick comments:
In `@src/functions/summarize.ts`:
- Around line 309-319: The comment above the retry loop in the summarize flow
references a fixed “2-attempt loop,” but the loop uses configurable
getMaxAttempts(). Update that comment to describe the shared/configurable retry
behavior without stating a hardcoded attempt count; leave the implementation
unchanged.
In `@src/observability/sentry.ts`:
- Around line 15-35: Export a flushSentry(timeoutMs) helper alongside initSentry
that returns immediately when Sentry is disabled, awaits Sentry.flush with the
provided timeout, and swallows flush errors. Update the shutdown handler in the
process lifecycle code to await flushSentry before calling process.exit(0).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56c7e477-b5bf-4eae-9607-14d7c9629c94
📒 Files selected for processing (4)
package.jsonsrc/functions/summarize.tssrc/index.tssrc/observability/sentry.ts
| release: process.env.AGENTMEMORY_COMMIT_SHA || undefined, | ||
| }); | ||
| enabled = true; | ||
| logger.info("Sentry initialized", { environment: process.env.FLY_APP_NAME }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Logged environment may not match the Sentry environment.
Line 28 logs process.env.FLY_APP_NAME as the environment, but the actual value sent to Sentry (line 23-24) could be NODE_ENV or "production" when FLY_APP_NAME is unset. This mismatch will cause confusion when correlating logs with Sentry environments.
🔧 Fix: extract environment to a variable and reuse
export function initSentry(): void {
const dsn = process.env.SENTRY_DSN;
if (!dsn) return;
try {
+ const environment =
+ process.env.FLY_APP_NAME || process.env.NODE_ENV || "production";
Sentry.init({
dsn,
tracesSampleRate: 0,
- environment:
- process.env.FLY_APP_NAME || process.env.NODE_ENV || "production",
+ environment,
release: process.env.AGENTMEMORY_COMMIT_SHA || undefined,
});
enabled = true;
- logger.info("Sentry initialized", { environment: process.env.FLY_APP_NAME });
+ logger.info("Sentry initialized", { environment });
} catch (err) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| logger.info("Sentry initialized", { environment: process.env.FLY_APP_NAME }); | |
| export function initSentry(): void { | |
| const dsn = process.env.SENTRY_DSN; | |
| if (!dsn) return; | |
| try { | |
| const environment = | |
| process.env.FLY_APP_NAME || process.env.NODE_ENV || "production"; | |
| Sentry.init({ | |
| dsn, | |
| tracesSampleRate: 0, | |
| environment, | |
| release: process.env.AGENTMEMORY_COMMIT_SHA || undefined, | |
| }); | |
| enabled = true; | |
| logger.info("Sentry initialized", { environment }); | |
| } catch (err) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/observability/sentry.ts` at line 28, In the Sentry initialization flow,
extract the resolved environment value used by the Sentry configuration into a
variable and reuse that variable in the “Sentry initialized” logger.info call.
Ensure the log matches the existing NODE_ENV, production fallback, and
FLY_APP_NAME resolution exactly.
Error messages from openai/openrouter/minimax/anthropic could embed raw HTTP response bodies or JSON-stringified provider responses, which then reached Sentry.captureException verbatim once error reporting was wired up. Log the full body/response locally only; throw fixed, content-free messages carrying just the status code. Extracted the shared pattern (openai/openrouter/minimax) into throwSafeHttpError/throwSafeShapeError in _fetch.ts. anthropic.ts uses the official SDK, whose APIError also embeds the raw body in .message -- wrapped every SDK call site in a new callSafely() helper that sanitizes before rethrowing.
… modes - initSentry() now checks Sentry.isInitialized() after Sentry.init() so a malformed SENTRY_DSN that silently no-ops doesn't get logged as a false "initialized" success. - Added flushSentry() so buffered events get a bounded chance to flush on shutdown instead of being dropped on process.exit. - captureFailure/captureException's internal try/catch now logs via the local logger when it swallows an SDK-level error, instead of silently swallowing with no trace. - captureException runs every error through a new toSafeError() that caps the forwarded message length as defense-in-depth (root-fix for the raw content is in the provider files, a separate commit on this branch).
…e failure classification - Both retry loops (chunk-level and top-level) now back off with jitter between attempts instead of retrying immediately. - SUMMARIZE_MAX_ATTEMPTS/SUMMARIZE_CHUNK_MAX_ATTEMPTS are clamped to a shared MAX_ATTEMPTS_CEILING=5 via a new getEnvInt() helper, replacing 4 near-duplicate env-parsing functions. - A new ChunkPartialCache persists resolved chunk summaries across top-level retry attempts, so a retry only re-runs the failed reduce/merge step instead of re-summarizing every already-succeeded chunk from scratch. - A chunk map-phase soft deadline (CHUNK_MAP_PHASE_DEADLINE_MS) stops starting new chunk batches once the map phase risks exceeding iii's 180s invocation timeout, turning a silent hard-timeout into a graceful partial skip subject to the existing MAX_SKIP_RATIO bailout. The deadline is a single absolute timestamp stored on ChunkPartialCache and shared across every top-level attempt -- NOT recomputed fresh per attempt, which would have let repeated retries each burn close to the full budget and blow past the 180s ceiling in aggregate. - The empty_provider_response/parse_failed Sentry classification now tracks outcomes across the whole retry loop (sawEmptyResponse/ sawParseFailure), not just the last attempt's state, so a real parse failure earlier in the loop isn't masked by a later empty response. - validation_failed now forwards only Zod error field paths, not the free-text messages, so a future schema change can't start leaking real values through this capture site unnoticed. - Added test/sentry.test.ts and a vi.mock for sentry.js in test/summarize.test.ts (previously real Sentry calls were possible if SENTRY_DSN were ever set in the test environment), plus new coverage for the env-var ceiling, mixed-failure classification, chunk-cache reuse, and the map-phase deadline. Also fixed two pre-existing test fixtures that assumed a hardcoded 2-attempt chunk retry count after an earlier commit on this branch bumped the default to 3 with no test update.
Also don't let a failed sdk.shutdown() prevent flushSentry() from running -- buffered events from the same shutdown sequence would otherwise be silently dropped.
…arball The Dockerfile previously installed either the published npm package (no way to ship a fork patch) or a manually-built, untracked tarball (no build-time check that it matched the checked-out source -- a fresh checkout with no local tarball would break, and a source change with no tarball rebuild would silently ship stale code). Replaced with a multi-stage build: a builder stage runs npm install + npm run build + npm pack against the checked-out source, and the final stage installs from that stage's output tarball. Build context moves from deploy/fly/ to the repo root (documented in fly.toml and the deploy README's fly deploy invocation) so the builder stage can see the source tree. Uses npm install rather than npm ci in the builder stage since this repo intentionally gitignores its lockfile. Added a root .dockerignore to keep the larger build context from uploading node_modules/docs/benchmark/etc. on every deploy.
…npm ci Addresses a P2 finding from this branch's own code review: the builder stage's npm install (no lockfile) meant two builds of the same commit could resolve different transitive dependency versions, with the locally-packed tarball as the sole artifact shipped to production. deploy/fly/package-lock.json is scoped ONLY to the Fly build context -- generated via npm install --package-lock-only against the current package.json and exempted from the repo-wide lockfile gitignore rule via a single !deploy/fly/package-lock.json negation. Local dev workflow and the root package-lock.json policy are untouched.
…e-retry.ts summarize.ts had grown to mix chunk splitting, XML parsing, function registration, AND all the retry/backoff/env-config/cache machinery in one file. Split the latter (getEnvInt and its 4 callers, sleep/ backoffDelayMs, MAX_ATTEMPTS_CEILING, ChunkPartialCache, the map-phase deadline constant) into a new summarize-retry.ts module. No behavior change -- pure extraction, re-exported and imported back in.
Previously initSentry()'s isInitialized() check (added this branch) was only ever visible via boot logs -- no way for a deploy-verification script or monitoring agent to confirm error reporting is actually live without tailing stdout at process start. Added isSentryEnabled() to observability/sentry.ts and wired it into HealthSnapshot.sentryEnabled, already exposed via the existing GET /agentmemory/health endpoint.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/functions/summarize-retry.ts (1)
1-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSeveral comments explain WHAT rather than WHY.
Per coding guidelines, TypeScript source should avoid comments explaining WHAT and rely on clear naming instead. The module-level comment (lines 1-5), the
getEnvIntdescription (lines 36-40), and thebackoffDelayMsparameter explanation (lines 96-98) describe what the code does rather than why it exists. The WHY comments (e.g., deadline rationale at lines 104-115, cache design at lines 117-127) are valuable and should be retained.As per coding guidelines: "In TypeScript source code, avoid code comments explaining WHAT — use clear naming instead."
Also applies to: 36-40, 96-98
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/functions/summarize-retry.ts` around lines 1 - 5, Remove the WHAT-only comments in the module header, the getEnvInt description, and the backoffDelayMs parameter explanation. Retain the WHY-focused comments covering deadline rationale and cache design, leaving the implementation unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/functions/summarize-retry.ts`:
- Around line 1-5: Remove the WHAT-only comments in the module header, the
getEnvInt description, and the backoffDelayMs parameter explanation. Retain the
WHY-focused comments covering deadline rationale and cache design, leaving the
implementation unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b88d4c87-a6f0-4754-8060-cc0e68e22421
📒 Files selected for processing (7)
src/functions/summarize-retry.tssrc/functions/summarize.tssrc/health/monitor.tssrc/observability/sentry.tssrc/types.tstest/health-thresholds.test.tstest/sentry.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/observability/sentry.ts
- test/sentry.test.ts
- src/functions/summarize.ts
flyctl resolves [build].dockerfile relative to the directory containing the config file (deploy/fly/), not the build context root. The prior value "deploy/fly/Dockerfile" doubled up to deploy/fly/deploy/fly/Dockerfile at build time, breaking every deploy. Verified via fly deploy --build-only against a throwaway app (destroyed after validation, zero cost): fails with the old value, succeeds with the corrected "Dockerfile".
- Fix a real deploy-blocking bug: $VOLUME was derived from $APP (agentmemory_<user>_data) but fly.toml hardcodes the mount source to agentmemory_data. Any reader using a non-'agentmemory' app name (which the doc itself tells them to do, since 'agentmemory' is taken) would create a volume that never attaches to the app. VOLUME is now a fixed literal matching fly.toml. - Drop the --dockerfile CLI flag from both fly launch/deploy examples; verified empirically this flyctl version ignores it entirely and only reads fly.toml's own [build].dockerfile field. - Correct the image size claim (~114 MB -> ~124 MB, measured this session) and the 512MB shared-cpu-1x monthly cost ($1.94 -> $3.69, per Fly's current pricing page). - Add the missing 'fly machine list' lookup step before both <machine-id> placeholders (rotate + restore sections), which were previously unresolvable dead ends. - Minor: cold-start/grace-period ratio is ~3.3x, not exactly 3x. Found via an independent two-reviewer pass (coherence + feasibility lenses) cross-checked against the actual fly.toml/Dockerfile/entrypoint.sh.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/fly/README.md`:
- Around line 151-153: Update the active Fly compute estimate in the pricing
section of README.md from $3.69/month to about $3.19/month, while preserving the
existing 24/7, shared-cpu-1x, 512 MB, and auto_stop_machines context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da83804b-1e21-45e8-b000-2e3f9dc03c5f
📒 Files selected for processing (2)
deploy/fly/README.mddeploy/fly/fly.toml
Comment on lines
151
to
+153
| - Active (machine running on `shared-cpu-1x` with 512 MB): about | ||
| $1.94/month if it ran 24/7; in practice `auto_stop_machines` keeps | ||
| that well under $1. | ||
| $3.69/month if it ran 24/7 (per Fly's current pricing page); in | ||
| practice `auto_stop_machines` keeps that well under $1. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section of the README
sed -n '130,170p' deploy/fly/README.md
# Look for the region and pricing references nearby
rg -n "iad|shared-cpu-1x|3\\.69|3\\.32|pricing" deploy/fly/README.mdRepository: rohitg00/agentmemory
Length of output: 2023
🌐 Web query:
Fly pricing shared-cpu-1x 512 MB iad monthly current pricing
💡 Result:
As of July 13, 2026, the monthly cost for a shared-cpu-1x machine with 512 MB of RAM on Fly.io is $3.19 [1][2][3]. This pricing is based on the resource usage rate of $0.0044 per hour [1]. Fly.io billing for compute resources is consistent across its regions, including iad (Northern Virginia), and is calculated based on the specific machine configuration [1][4]. Please note that while individual resource usage is billed at these rates, your account may also be subject to the costs associated with the specific Fly.io plan you are subscribed to (e.g., Hobby or Launch plans), which may have separate monthly base fees [3].
Citations:
- 1: https://fly.io/docs/about/pricing/
- 2: https://community.fly.io/t/heroku-dyno-equivalents-on-fly/3428
- 3: https://community.fly.io/t/free-plan-clarification/18661
- 4: https://fly.io/docs/about/billing/
Update the Fly compute estimate. The current shared-cpu-1x 512 MB rate is about $3.19/month, and Fly compute pricing is region-independent, so the $3.69/month figure is outdated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/fly/README.md` around lines 151 - 153, Update the active Fly compute
estimate in the pricing section of README.md from $3.69/month to about
$3.19/month, while preserving the existing 24/7, shared-cpu-1x, 512 MB, and
auto_stop_machines context.
Source: MCP tools
The builder stage never copied tsdown.config.ts, so tsdown silently
fell back to a single-entry build (src/index.ts only) instead of the
config's 3-target + 13-hook multi-entry build. The packed tarball's
dist/ was missing cli.mjs entirely, so package.json's bin field
(agentmemory -> dist/cli.mjs) pointed at a file that never existed.
npm therefore skipped creating the node_modules/.bin/agentmemory
symlink, and the container crash-looped on every boot with
'exec: "agentmemory": executable file not found in $PATH'
(exit code 1, immediately hit Fly's 10-restart cap).
Caught by running the full deploy runbook end-to-end (fly launch +
volume create + real fly deploy, not just --build-only) against a
throwaway app -- the build-only dry run can't catch this since tsdown
exits 0 either way, it just silently builds less than intended.
Verified fixed: redeployed, /agentmemory/livez returned 200 with
{"status":"ok"}, machine health check passing. Test app + volume
destroyed after verification.
Sentry.init() replaces the global client on every call rather than merging options. Production runs with NODE_OPTIONS='--require /opt/agentmemory/sentry-preload.cjs', an out-of-band preload script that already calls Sentry.init() with its own tracesSampleRate and release before this module ever runs. Calling init() again here (as this module unconditionally did) would silently wipe that configuration -- verified empirically: a second init() call creates a brand-new client instance and drops the first call's tracesSampleRate/release entirely. initSentry() now checks Sentry.isInitialized() before calling init(), and reuses an already-initialized client (just flips the local enabled flag) instead of re-initializing over it. Added a test for this exact scenario; updated four existing tests whose isInitialized() mock modeled only the post-init state to also model the pre-init check this adds.
Production's NODE_OPTIONS references '--require /opt/agentmemory/sentry-preload.cjs' for broad crash reporting that runs before this package's own initSentry() gets a chance to. That file existed only in the currently-deployed production image via some process outside this git repo -- not in main, not in this branch, nowhere tracked. Deploying this fork's rebuilt image without it caused a real production outage (~7 min, ryclar-agentmemory): the process crashed on every boot with 'Cannot find module .../sentry-preload.cjs', hit Fly's restart cap, and went fully down. Rolled back to the prior image immediately to restore service; no data was lost (deploy only replaces the app image, never touches /data). Root-cause fix: the preload script is now a tracked file (deploy/fly/sentry-preload.cjs, content matches what was already running in production) baked into the image via the Dockerfile, so this fork's own build is self-contained and reproducible instead of depending on an undocumented prior manual patch. Verified in a fresh throwaway app with NODE_OPTIONS set to the same --require path: 'Sentry disabled: SENTRY_DSN not set' logs cleanly (no crash), engine boots, health check passes. Composes correctly with the double-init fix in the previous commit (initSentry() reuses the client this preload creates instead of re-initializing over it).
The endpoint accepted `limit` but never read it: every request did an
unconditional full kv.list() and then hydrated a summary for every session
in the store. At ~10k sessions that is a ~13MB response, which is what
breaks the handoff/session-history skills -- they ask for 20 and get all of
them. The MCP proxy has always sent limit=20 (src/mcp/standalone.ts:149),
so this was purely a server-side omission; the Cloudflare gateway had
already added client-side defensive truncation to work around it.
Two independent paths served memory_sessions and both ignored limit:
- api::sessions (REST; reached via standalone.ts and the gateway)
- server.ts's in-process MCP (reached via POST /agentmemory/mcp/call)
Fixing only the first would have left the connector path unbounded.
Absent `limit` still returns every session, deliberately.
scripts/agentmemory-import-with-skip.py builds its already-imported
skip-set from a single unbounded call and implements no pagination loop by
design; a default cap would silently shrink that set and re-import the
remainder as duplicates. Explicit limits are honored as given rather than
clamped, so summarize-backfill.py's bulk fetch keeps working.
Ordering is load-bearing: sort newest-first BEFORE slicing, or a limit
returns an arbitrary N instead of the recent sessions callers expect. Page
before hydrating summaries to drop the N+1 (one kv.get per returned
session, not one per session in the store). Response now carries the
unpaged `total` so a bounded reply is not mistaken for the whole store.
memory_sessions' inputSchema was `properties: {}`, which advertises a
tool that takes no arguments -- limit was undiscoverable to MCP clients.
Tests assert against each handler body rather than the whole file: a
file-wide regex for `limit` also matches api::commits and would pass
vacuously. All 7 fail on the pre-fix source.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mem::summarizefailures were only written to the structuredlogger, so they never surfaced in Sentry even when a DSN was configured — the engine imports no@sentrySDK. In a deployment with a lifetime ~20.7% summarize failure rate (empty/unparseable/schema-invalid LLM output, plus thrown provider errors), the error tracker showed nothing, making the failures effectively invisible.This PR wires the four
mem::summarizefailure sites into optional Sentry reporting and makes the retry counts configurable.Changes
1. Optional Sentry reporting (
src/observability/sentry.ts, new)SENTRY_DSNis set — builds, tests, and deploys without a DSN are completely unaffected.initSentry()called once at worker startup insrc/index.ts.registerSummarizeFunctionwith tags (provider, mode, observationCount, latency):empty_provider_response,parse_failed,validation_failed→captureMessage(warning)captureException2. Configurable retries
2→SUMMARIZE_MAX_ATTEMPTS(default 3). A third attempt recovers a meaningful fraction of empty/parse failures from LLMs that intermittently return empty or wrapped output.summarizeChunkWithRetry: hard2→SUMMARIZE_CHUNK_MAX_ATTEMPTS(default 3), kept as a separate knob because chunk retries multiply across every chunk and concurrency slot; ops can tune it down independently under provider throttling. The existingMAX_SKIP_RATIObailout still guards against half-blind merges.Why opt-in
@sentry/nodeis added as a dependency, but nothing initializes or calls Sentry unlessSENTRY_DSNis present in the environment. Zero behavior change for existing users who don't set it.Testing
tsc --noEmit: 0 errors in the changed files (summarize.ts,sentry.ts,index.ts).npm run build(tsdown): passes; new env knobs (SUMMARIZE_MAX_ATTEMPTS,SUMMARIZE_CHUNK_MAX_ATTEMPTS) andinitSentry/captureFailureconfirmed present in the bundle.mem::summarizeinvocations across production sessions succeeded post-build with no regressions.New env vars (all optional)
SENTRY_DSNSUMMARIZE_MAX_ATTEMPTS3SUMMARIZE_CHUNK_MAX_ATTEMPTS3Summary by CodeRabbit