fix: port battle-tested runtime fixes from long-lived deployment - #991
fix: port battle-tested runtime fixes from long-lived deployment#991TheGeekFreaks wants to merge 2 commits into
Conversation
- default extraction model: gpt-oss-120b is served via groq/, the openai/ provider path never resolved - bootstrap entry gets a required "name": core bootstrap-budget calls name.toLowerCase() unguarded on overflow and killed every agent turn - legacy memories: run the upgrade automatically at startup (noLlm, bounded batches) instead of logging a manual instruction nobody runs; skip entries with an empty id (endless per-run error spam otherwise) - embedder: precise context-error detection and a 512-char minimum before auto-chunking (prevents infinite reduction loop on auth or network errors) - store: orphaned-lock threshold 5min -> 30s (measured 41s recall stalls) and lock retry minTimeout 1s -> 50ms (lock is held ~200ms)
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The single-embed guard variable does not exist in the batch retry path; use the aggregate batch length, which is also the semantically correct bound there (providers reject the aggregate, not the items).
rwmjhb
left a comment
There was a problem hiding this comment.
The production failures described here are worth fixing, and the bootstrap name addition plus the general lock/embedder hardening direction make sense. I reviewed head d3ba0d0, however, and the current branch has several release and data-safety blockers.
-
The published package contains none of these fixes. This PR changes four source files but no
dist/files, whilepackage.json.mainisdist/index.js. Runningnpm run buildon this head succeeds and produces uncommitted changes indist/index.js,dist/src/embedder.js,dist/src/memory-upgrader.js, anddist/src/store.js(57 insertions, 17 deletions). Please commit the generated runtime and run the package-runtime verifier. -
The new 512-character guard breaks an existing aggregate-batch fallback.
node --test test/embedder-max-input-chars.test.mjsfails the existing “batch context fallback retries individual items before chunking” case: two individually valid short inputs receive an authoritative aggregate HTTP 413, butsrc/embedder.ts:1309suppresses the per-item retry because their total is below 512 and rethrows instead. Preserve individual fallback for authoritative aggregate context responses; apply the size heuristic only to ambiguous message-only errors. -
Changing the model string does not route the client through Groq.
index.ts:2630and:3528now choosegroq/openai/gpt-oss-120b, but the API-key client sendsconfig.modelunchanged to the existing OpenAI-compatiblebaseURL; no provider routing accompanies the prefix. The manifest, UI placeholder, README, and CLI status still declareopenai/gpt-oss-120b, so host schema materialization can also bypass the new fallback entirely. Resolve provider and direct-API model IDs centrally, keep every default surface consistent, and add outbound request-shape tests for default and custom base URLs. -
The automatic startup migration is not safe as an unconditional background write. Five seconds after startup, before the first backup scheduled at 60 seconds, every installation may rewrite up to 500 rows with heuristic
noLlmmetadata. Stampingmemory_categorymakes those rows ineligible for the normal LLM-backed upgrade path. The patch is also prepared from the initial list snapshot and later replaces the complete metadata string, so live access counters, tiers, or suppression updates committed between scan and write can be reverted. Make this opt-in or explicitly version/resume heuristic upgrades, back up before mutation, and re-read/merge current metadata under the write lock. -
The claimed cross-start drain and lifecycle handling are incomplete. Both counting and upgrade inspect only the newest 10,000 rows; upgraded rows remain in that fixed window, so older legacy rows are never reached. The 500-row limit is applied before empty IDs are removed, allowing unusable rows to consume every run. The mutating five-second timer is also not retained or cancelled on stop. Add pagination/cursor progress, filter or quarantine unusable rows before limiting, and cancel/guard the migration during shutdown and restart.
No GitHub checks are currently reported and this PR adds no tests. Locally, the lock, upgrader, bootstrap, and API-key client suites pass, but the changed embedder regression fails and therefore the full suite is red. Please add focused coverage for every new branch above and rerun the complete suite after rebuilding dist.
Ports five fixes that accumulated in a long-lived production deployment of this plugin (single-operator gateway, ~5k memories, several weeks of runtime). Each one addresses a failure that was observed and measured live, details in the code comments.
Changes
gpt-oss-120bis served viagroq/; theopenai/provider path never resolved, so smart extraction silently ran without a working model.namefield: the core bootstrap-budget checker callsname.toLowerCase()unguarded oncebootstrapTotalMaxCharsoverflows — an entry withoutnamekilled every agent turn before the model call ("Embedded agent failed before reply").virtual: truedoes not protect against it./context|too long|exceed|length/), so auth/network errors on short strings triggered an infinite chunk-reduction loop. Tightened regex + 512-char minimum before chunking.minTimeout1s → 50ms (the lock is really held ~200ms; three concurrentpatchMetadatacalls burned ~3s of the 8s recall budget idle-waiting).Test plan
🤖 Generated with Claude Code