This repository was archived by the owner on Oct 12, 2026. It is now read-only.
Repository navigation
fix(memory): audit follow-up — ranking, storage-layer and concurrency defects - #1
Merged
Merged
Conversation
Ranking and reporting - Guard unparseable timestamps in daysBetween, fromMetadata and parseDelta. A NaN score makes sort's comparator return NaN, which abandons the ordering of the whole result set rather than misplacing the one bad entry. - Drop the denominator from recall's header: hits.length is the candidate neighbourhood the query pulled, not how many memories the project has. - Report memory_stats as a lower bound past the scan cap instead of presenting a truncated count as exact, and describe what the tool actually measures. - Gate against the strongest hit rather than the first. VectorStore.query promises nearest neighbours, not an order, and understating the top loosens every gate below it. Model-independence - Show standing against the top result instead of a raw cosine. What a cosine means belongs to the embedding model, so "30% match" beside HIGH CONFIDENCE contradicts itself on Titan and means something else under another model. - Require word overlap as well as cosine before remember declines to write. Declining is silent, so it must not turn on a number a model swap changes. - Correct the claim that RECALL_MIN_SIMILARITY is the only model-specific value, in the README and in two docstrings that repeated it. Operability - Add log.ts and report the failures previously swallowed into tool results, so an outage reaches the pod log instead of only a model's context. Memory content and queries cannot enter the log by construction. - Flush counters on shutdown even when server.close never calls back. - Answer an oversized body with 413 rather than a parse error, and refuse JSON-RPC batches out loud rather than replying 202 to a request that expects a response. - Treat forget's index cleanup as garbage collection: the vector is already gone, so a failed cleanup is not a failed deletion. - Refuse RECALL_MIN_SIMILARITY=0, which labelled every result HIGH CONFIDENCE. - Freeze EMPTY_STATS, which every untouched memory shares. - Run the container as the unprivileged node user.
📝 WalkthroughWalkthroughThe change updates container permissions, configuration validation, structured error logging, request handling, memory ranking and deduplication, statistics integrity, and graceful shutdown behavior. ChangesRuntime and memory reliability
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Process
participant gracefulShutdown
participant HTTPServer
participant StatisticsStore
Process->>gracefulShutdown: receive SIGTERM or SIGINT
gracefulShutdown->>HTTPServer: close server
gracefulShutdown->>StatisticsStore: flush statistics
gracefulShutdown->>Process: exit with code 0
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
remember could be made to fail with a rejection from AWS naming neither the field nor the limit, using nothing but arguments a model chose: - `category` lands in the filterable half and can exhaust its 2 KB alone. A 3 KB category serialised to 3,060 bytes and the write was refused. - Twenty tags land beside a body that may be 32 KB, against a 40 KB total. Max content with 20 tags of 500 bytes serialised to 42,196. Only `content` was checked, which is the one field of the three that a caller was already being told about. assertWithinMetadataBudget now measures the serialised metadata, both halves, and names the half that overflowed. The tool layer gains byte ceilings for category and tags so the model is told which field to shorten before the store has to, and both appear in the schema descriptions. Also fetch counter shards in bounded batches instead of one at a time. This runs on the critical path of every recall whose cache has expired, and a shard is a few hundred bytes — serialising the round trips spent most of a recall's latency waiting on nothing. The merge is order-independent by construction, so only the fan-out needed bounding.
Checked the code's assumptions against the published S3 Vectors limits. The per-call chunking was already right (GetVectors 100, DeleteVectors 500), but three things were not: - topK was passed through unclamped. A QueryVectors response carries at most 100 results and the rest sit behind a nextToken this store does not follow, so a larger topK is answered with a silent prefix rather than an error. It is now clamped in the store, where the constant cannot drift away from a caller. - The filterable budget was taken as 2048. AWS documents "2 KB" without saying which K, so it is now 2000: being wrong that way refuses a memory the service would have taken, and being wrong the other way lets through the rejection the check exists to pre-empt. - query carried a memoryType filter parameter no caller has ever passed. It built a two-key filter whose implicit-AND semantics the documentation does not state, on the line that separates one project's memories from another's. Removed rather than left as an untested branch on that boundary; the single bare key that remains is documented as an implicit $eq. Adds vectors.test.ts. The real store had no tests at all — the fake stands in for it everywhere above this layer, so nothing asserted what the tenant filter actually looks like on the wire, or that a record naming another tenant is dropped even if the filter were wrong.
DeleteObjects answers 200 and names the keys it could not remove in `Errors`. The store awaited the call and discarded the response, so a delete that removed nothing resolved as a success. Both callers treat deletion as best-effort cleanup and log when it fails — forget's index entry, and the shards compaction has already absorbed. That made this the one signal that cleanup is broken, and it was the one being thrown away: the log lines added for exactly this could never fire on its most likely cause, while orphaned index keys inflated memory_stats and absorbed shards accumulated unbounded. Adds objects.test.ts. Like the vector store, the real object store had no tests — and it carries the conditional-write headers that are the whole reason compaction is safe across pods, verified until now only through a fake that reimplements the rule rather than against the code that asks S3 for it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/server.ts (1)
180-216: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winGuard against a
nullJSON-RPC body; it crashes the process.
JSON.parse("null")returns the JavaScript valuenull, not an error.Array.isArray(null)isfalse, so the new batch check does not catch it. Execution reachesmessage.id === undefinedon Line 213, which reads a property offnulland throws aTypeError.That throw happens inside the
void (async () => { ... })()wrapper, so it becomes an unhandled promise rejection. Node terminates the process on an unhandled rejection by default. A single POST with bodynullcrashes the server for every tenant, and the README documents the unauthenticated mode as the intended deployment, so any caller that reaches/mcpcan trigger this.Add a check for a non-object message alongside the new batch check.
🐛 Proposed fix
- // A JSON-RPC batch. Refused rather than answered — this protocol revision + // A malformed or batched request. Refused rather than answered — this + // protocol revision removed batching, and `null`/non-object JSON has no + // `id` to reply to, so both must be rejected before the notification + // check below reads a property off them. + if (!message || typeof message !== "object") { + send(response, 400, { + jsonrpc: "2.0", + id: null, + error: { code: INVALID_REQUEST, message: "request must be a JSON object" }, + }); + return; + } + // A JSON-RPC batch. Refused rather than answered — this protocol revision // removed batching — but refused *out loud*: an array has no `id`, so the // notification branch below would take it for a notification and reply // 202, leaving a client that sent one waiting for a response that is // never coming. if (Array.isArray(message)) {Add a regression test in server.test.ts alongside the batch test, sending body
nulland asserting the server stays up and returns 400. Do you want me to generate that test?🤖 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/server.ts` around lines 180 - 216, Guard the parsed message before property access in the request handler around the existing Array.isArray batch check: reject null and other non-object JSON bodies with the same 400 INVALID_REQUEST response used for unsupported batches, while preserving valid object and batch handling. Add a regression test in server.test.ts alongside the batch test that posts null, asserts a 400 response, and verifies the server remains available.src/service.ts (1)
445-466: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFetching exactly
STATS_SCAN_CAPkeys cannot distinguish "exactly at the cap" from "more than the cap".
objects.list(...)at Line 446 requests at mostSTATS_SCAN_CAPkeys. The check at Line 461,keys.length >= STATS_SCAN_CAP, is equivalent tokeys.length === STATS_SCAN_CAPhere, since the list call never returns more than that. When a tenant has exactlySTATS_SCAN_CAPmemories and no more, the scan is not actually truncated, but the response still says "At least ... a lower bound," which is misleading for that exact boundary.Request one extra key to detect truncation before reporting a lower bound.
🔧 Proposed fix to disambiguate the exact-cap case
async stats(tenant: string): Promise<string> { - const keys = await this.objects.list(`index/${tenant}/`, STATS_SCAN_CAP); + const keys = await this.objects.list(`index/${tenant}/`, STATS_SCAN_CAP + 1); + const truncated = keys.length > STATS_SCAN_CAP; + const scanned = truncated ? keys.slice(0, STATS_SCAN_CAP) : keys; const counts = new Map<string, number>(); - for (const key of keys) { + for (const key of scanned) { const entry = parseIndexKey(key); if (entry) { counts.set(entry.memoryType, (counts.get(entry.memoryType) ?? 0) + 1); } } const total = [...counts.values()].reduce((a, b) => a + b, 0); if (total === 0) { return "[MEMORY] This project has no memories yet."; } const breakdown = MEMORY_TYPES.filter((type) => counts.has(type)) .map((type) => `${type}: ${counts.get(type)}`) .join(", "); - if (keys.length >= STATS_SCAN_CAP) { + if (truncated) { return ( `[MEMORY] At least ${total} memories for this project (${breakdown}). ` + `Counting stopped at ${STATS_SCAN_CAP} keys, so every figure here is a lower bound.` ); } return `[MEMORY] ${total} memories for this project (${breakdown}).`; }🤖 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/service.ts` around lines 445 - 466, Update stats to request STATS_SCAN_CAP + 1 keys from objects.list, then treat the result as truncated only when keys.length exceeds STATS_SCAN_CAP. Limit counting to the first STATS_SCAN_CAP entries so the extra probe key is not included, while preserving the exact-count response when no extra key is returned.
🤖 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 `@README.md`:
- Around line 229-246: Replace the indented command examples in the README’s
Bedrock and OpenAI usage sections with fenced code blocks, preserving their
commands and line continuation exactly. Ensure each block has matching opening
and closing fences so markdownlint MD046 passes.
In `@src/shutdown.ts`:
- Around line 60-75: Update flushAndExit in the shutdown flow to defer
options.exit(0) by one event-loop turn after stats.stop() settles, including
when the flush rejects, so synchronously logged failure output can drain before
process exit. Preserve the existing once-only flushed guard and rejection
handling.
---
Outside diff comments:
In `@src/server.ts`:
- Around line 180-216: Guard the parsed message before property access in the
request handler around the existing Array.isArray batch check: reject null and
other non-object JSON bodies with the same 400 INVALID_REQUEST response used for
unsupported batches, while preserving valid object and batch handling. Add a
regression test in server.test.ts alongside the batch test that posts null,
asserts a 400 response, and verifies the server remains available.
In `@src/service.ts`:
- Around line 445-466: Update stats to request STATS_SCAN_CAP + 1 keys from
objects.list, then treat the result as truncated only when keys.length exceeds
STATS_SCAN_CAP. Limit counting to the first STATS_SCAN_CAP entries so the extra
probe key is not included, while preserving the exact-count response when no
extra key is returned.
🪄 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 Plus
Run ID: febe1ed6-00c4-4d60-aac2-eedc69fecbba
📒 Files selected for processing (19)
DockerfileREADME.mdsrc/config.test.tssrc/config.tssrc/log.tssrc/main.tssrc/ranking.test.tssrc/ranking.tssrc/server.test.tssrc/server.tssrc/service.test.tssrc/service.tssrc/shutdown.test.tssrc/shutdown.tssrc/store/stats.test.tssrc/store/stats.tssrc/store/vectors.tssrc/tools.tssrc/types.ts
The watermark stops a shard being counted twice. Nothing stopped one being counted zero times. A shard's key is stamped when the flush builds it, not when S3 accepts it. A compaction that lists between those two moments draws the line above a shard that has not landed; when it does land it is already underneath, so its counts are dropped and the object stays in the prefix forever — listed on every read, absorbed by nothing, counted by no one. Clock skew turns that race into a pattern. Each pod stamps ULIDs from its own clock, so one running behind mints keys that sort low against everyone else's and can lose most of what it flushes rather than the occasional shard. Compaction now absorbs only shards older than COMPACTION_LAG_MS. Younger ones are still counted, but the line is not drawn past them, and they are kept out of the merged copy so the next read does not add them twice. The guarantee is now: any write landing within a minute of being stamped is counted, clock skew included. Three existing compaction tests had their clocks advanced past the lag. Two of them had started passing without compaction running at all, which made their assertions vacuous rather than wrong.
nalbam
changed the title
Two statements had gone stale, both from changes made earlier on this branch: - STATS_COMPACT_THRESHOLD was described as a count of shards. It is compared against settled shards only — the ones older than the compaction lag — so shards inside the lag are counted but neither absorbed nor counted toward triggering a fold. - RECALL_MIN_SIMILARITY was still called "the one number here that belongs to the embedding model", which the section two below it contradicts by documenting a second one. The docstring had been corrected and the prose had not. Adds the limits a caller can actually hit — content, category, tags, limit, and the memory_stats scan cap. They existed only in the tool schemas, which the model reads and an operator debugging a refused remember does not. Also records that a zero floor is refused at boot rather than accepted.
There was a problem hiding this comment.
Pull request overview
This pull request is an audit follow-up that hardens the memory server’s correctness and operability: it fixes ranking/dedup/reporting behaviors that could silently mislead the model, and it closes storage-layer and counter-compaction races that could silently lose data or cleanup signals.
Changes:
- Makes ranking/reporting model-independent and robust to bad timestamps (avoids NaN-driven sort failures; uses relative standing; fixes recall totals and stats truncation messaging; avoids assuming vector-store ordering).
- Enforces storage/service quotas and correctness at the edges (metadata-budget accounting, topK clamping, S3 DeleteObjects per-key failure handling), backed by new store-level tests.
- Improves operability and shutdown safety (structured error logging, graceful shutdown with flush-on-timeout, explicit 413 and batch refusal in JSON-RPC transport, container runs as unprivileged user).
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Freezes EMPTY_STATS to prevent shared-object mutation affecting multiple reads. |
| src/tools.ts | Adds label byte ceilings; improves tool descriptions and logs unexpected tool failures. |
| src/tools.test.ts | Tests category/tag byte-limit enforcement for remember. |
| src/store/vectors.ts | Clamps topK, removes unused filter branch, validates metadata budgets by serialized bytes, guards timestamps. |
| src/store/vectors.test.ts | New tests asserting real S3 Vectors command inputs and quota-related behavior. |
| src/store/stats.ts | Adds compaction lag, batched shard reads, timestamp coercion, and logs previously swallowed failures. |
| src/store/stats.test.ts | New tests for compaction lag, batching, frozen empty stats, and dropping unparseable timestamps. |
| src/store/objects.ts | Treats DeleteObjects per-key failures as errors instead of silently succeeding. |
| src/store/objects.test.ts | New tests asserting S3 object-store command inputs and error handling (ETag CAS, delete failures, listing). |
| src/shutdown.ts | Extracts graceful shutdown logic that flushes counters even if server.close never completes. |
| src/shutdown.test.ts | New tests covering shutdown ordering, timeout flush, idempotency on repeated signals, and flush failure. |
| src/service.ts | Fixes recall gating assumptions, improves recall/remember output semantics, and treats index cleanup as GC. |
| src/service.test.ts | Adds coverage for dedup independence from cosine alone, recall reporting fixes, timestamp robustness, and stats truncation messaging. |
| src/server.ts | Adds structured logging on unexpected request failures; returns 413 for oversized payloads; refuses JSON-RPC batches explicitly. |
| src/server.test.ts | Adds tests for server version identity, 413 behavior, and batch refusal. |
| src/ranking.ts | Prevents NaN in elapsed-days math; adds relativeStanding; clarifies confidence guidance invariants. |
| src/ranking.test.ts | Adds tests ensuring unreadable timestamps cannot break sorting; tests relativeStanding. |
| src/main.ts | Wires graceful shutdown with a bounded grace period for flushing counters. |
| src/log.ts | Introduces structured stderr JSON logging with restricted context fields. |
| src/config.ts | Refines config docs; refuses RECALL_MIN_SIMILARITY=0 as an invalid deployment setting. |
| src/config.test.ts | New tests covering config validation and RECALL_MIN_SIMILARITY constraints. |
| README.md | Updates tool semantics, documents compaction lag, clarifies model-specific values, and adds run examples/logging notes. |
| Dockerfile | Runs the container as the unprivileged node user. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+30
to
+37
| export interface ShutdownOptions { | ||
| /** How long to let in-flight requests finish before flushing anyway. */ | ||
| graceMs: number; | ||
| exit: (code: number) => void; | ||
| /** Injected in tests; defaults to the global. */ | ||
| setTimer?: (handler: () => void, ms: number) => { unref?: () => void }; | ||
| clearTimer?: (handle: never) => void; | ||
| } |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
Changes
1. Ranking and reporting (
a4e8fe1)NaNscore makessort's comparator returnNaN, which abandons the ordering rather than misplacing the one bad entry. Guarded at both parse boundaries and indaysBetween.recallreported a total it never counted —5 of 30 memories, where30was the candidate neighbourhood, not the project's memory count.memory_statspresented a truncated count as exact past its scan cap, and its description described background counter aggregation the tool never touches.VectorStore.querypromises nearest neighbours, not an order; understating the top loosens every gate below it.2. Model-independence (
a4e8fe1)30% matchbesideHIGH CONFIDENCEcontradicts itself on Titan. Now a standing against the top result, computed from the composite score so it cannot run backwards down the displayed list.rememberdeclined to write on the cosine alone. Declining is silent, so it must not turn on a number a model swap changes; it now also requires the two texts to share half their words.RECALL_MIN_SIMILARITYis the only model-specific value — in the README and two docstrings that repeated it.3. Operability (
a4e8fe1)log.ts— storage errors, embedding timeouts and four best-effort.catch(() => {})sites became text for the model and left the pod log empty.LogContextaccepts onlytenant/tool/method/key, so memory content and queries cannot enter the log by construction.server.closenever calls back, extracted toshutdown.tsbecausemain.tsruns on import and is otherwise only testable by signalling a process.413for an oversized body, and JSON-RPC batches refused explicitly rather than matching the no-idnotification branch and returning202to a sender that is waiting.forget's index cleanup is garbage collection — the vector is already gone, so a failed cleanup is not a failed deletion.RECALL_MIN_SIMILARITY=0refused (it labelled every resultHIGH CONFIDENCE);EMPTY_STATSfrozen; container runs as the unprivilegednodeuser.4. Metadata budget (
e31d891)remembercould be made to fail with a rejection from AWS naming neither the field nor the limit, using only arguments a model chose. Measured:categorycontent+ 20 tags × 500 BOnly
contentwas checked.assertWithinMetadataBudgetnow measures the serialised metadata, both halves, and names the half that overflowed; the tool layer gained byte ceilings so the model is told which field to shorten first.Also fetches counter shards in bounded batches rather than one at a time — this runs on the critical path of every recall whose cache has expired.
5. Documented service quotas (
9534978)Checked the code's assumptions against the published S3 Vectors limits. Per-call chunking was already correct; three things were not.
topKwas passed through unclamped. AQueryVectorsresponse carries at most 100 results and the rest sit behind anextTokenthis store does not follow — so a largertopKis answered with a silent prefix, not an error.querycarried amemoryTypefilter no caller has ever passed, building a two-key filter whose implicit-AND semantics the documentation does not state — on the line separating one project's memories from another's. Removed rather than left as an untested branch on that boundary.Adds
vectors.test.ts: the real store had no tests, so nothing asserted what the tenant filter looks like on the wire.6. Storage-layer and concurrency defects (
42f7aea,a05e057)DeleteObjectsanswers200and names failed keys in the body. The store discarded the response, so a delete that removed nothing resolved as success — which made the two cleanup log lines above unable to fire on their most likely cause, while orphaned index keys inflatedmemory_statsand absorbed shards accumulated unbounded. Addsobjects.test.ts, covering the conditional-write headers that are the whole reason compaction is safe across pods.COMPACTION_LAG_MS.7. Documentation sync (
33de6fe)Two README statements had gone stale against changes made earlier on this branch:
STATS_COMPACT_THRESHOLDis compared against settled shards only, andRECALL_MIN_SIMILARITYwas still called the only model-specific number in a document that goes on to describe a second one. Also records the limits a caller can actually hit —content,category,tags,limit, and thememory_statsscan cap — which existed only in the tool schemas that the model reads and an operator debugging a refusedrememberdoes not.Breaking Changes
None to the wire protocol or the storage format. Three behavioural changes worth knowing at deploy time:
RECALL_MIN_SIMILARITY=0now stops the process at boot. Any deployment setting it must set a real floor.remembernow refuses acategoryover 128 bytes or a tag over 64 bytes with a tool error, where it previously attempted the write.recallandremember. Nothing parses these — the consumer is a model — but transcripts will look different.Test Plan
npm run typecheck,npm test(180 passing; 116 before this branch),npm run buildtsc --noUnusedLocals --noUnusedParameterscleanECR/503that carried signal through an ASCII tokenizer; and two compaction tests that had stopped running compaction at all once the lag landed.uid=1000(node),/appnot writable,tools/listanswers, an oversized category is refused before any AWS call, an unreachable Bedrock surfaces as a tool error rather than a crash, that failure appears in the pod log as one JSON line, the recall query does not appear in the log, andSIGTERMexits 0.recall/rememberoutput changes read acceptably in Agent Studio transcripts.RECALL_MIN_SIMILARITY=0.Risks and limitations
service.tsalone carries seven distinct fixes — so it is split by commit rather than by file. Reviewing commit by commit is the intended path.COMPACTION_LAG_MS(60s) andDEDUP_MIN_WORD_OVERLAP(0.5) are new judgement calls. The lag must exceed worst-case clock skew plus slowest PUT; a minute is far more than either, and the cost of it being too small is lost counts, not wrong ones. The overlap sits in a wide gap and errs toward writing a duplicate rather than dropping a memory.memory_statsstill makes up to 10 sequentialListObjectsV2calls at its cap. Fixing that means moving the type into the index key prefix — a migration of existing keys, and unfiltered newest-first listing becomes a 4-way merge. At realistic scale it is a single call.