Skip to content

fix: port battle-tested runtime fixes from long-lived deployment - #991

Open
TheGeekFreaks wants to merge 2 commits into
CortexReach:masterfrom
TheGeekFreaks:port/local-runtime-fixes
Open

fix: port battle-tested runtime fixes from long-lived deployment#991
TheGeekFreaks wants to merge 2 commits into
CortexReach:masterfrom
TheGeekFreaks:port/local-runtime-fixes

Conversation

@TheGeekFreaks

Copy link
Copy Markdown

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

  • Default extraction model provider: gpt-oss-120b is served via groq/; the openai/ provider path never resolved, so smart extraction silently ran without a working model.
  • Bootstrap entry name field: the core bootstrap-budget checker calls name.toLowerCase() unguarded once bootstrapTotalMaxChars overflows — an entry without name killed every agent turn before the model call ("Embedded agent failed before reply"). virtual: true does not protect against it.
  • Legacy memory auto-upgrade: the startup check only counted legacy entries and logged a manual instruction. Observed result: 54% of entries (2527 of 4713) permanently stuck without smart metadata. Now upgrades automatically (noLlm, bounded batches so startup is never blocked); entries with an empty id are skipped instead of producing ~2700 identical errors per day.
  • Embedder auto-chunking guard: the context-error regex matched almost any error message (/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.
  • Store lock tuning: orphaned-lock threshold 5min → 30s (measured 41s autoRecall stalls on a stale lock) and retry minTimeout 1s → 50ms (the lock is really held ~200ms; three concurrent patchMetadata calls burned ~3s of the 8s recall budget idle-waiting).

Test plan

  • All five fixes running in production on the source deployment (2026-07 → 2026-08) without regressions
  • CI

🤖 Generated with Claude Code

- 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)
@chatgpt-codex-connector

Copy link
Copy Markdown

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 rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. The published package contains none of these fixes. This PR changes four source files but no dist/ files, while package.json.main is dist/index.js. Running npm run build on this head succeeds and produces uncommitted changes in dist/index.js, dist/src/embedder.js, dist/src/memory-upgrader.js, and dist/src/store.js (57 insertions, 17 deletions). Please commit the generated runtime and run the package-runtime verifier.

  2. The new 512-character guard breaks an existing aggregate-batch fallback. node --test test/embedder-max-input-chars.test.mjs fails the existing “batch context fallback retries individual items before chunking” case: two individually valid short inputs receive an authoritative aggregate HTTP 413, but src/embedder.ts:1309 suppresses 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.

  3. Changing the model string does not route the client through Groq. index.ts:2630 and :3528 now choose groq/openai/gpt-oss-120b, but the API-key client sends config.model unchanged to the existing OpenAI-compatible baseURL; no provider routing accompanies the prefix. The manifest, UI placeholder, README, and CLI status still declare openai/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.

  4. 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 noLlm metadata. Stamping memory_category makes 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.

  5. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants