fix(register): move _registeredApis.add after successful _initPluginState (Must-Fix #1)#702
fix(register): move _registeredApis.add after successful _initPluginState (Must-Fix #1)#702jlin53882 wants to merge 2 commits intoCortexReach:masterfrom
Conversation
…tate Resolves reviewer Must-Fix #1 from PR CortexReach#522: - _registeredApis.add(api) was called BEFORE _initPluginState. If _initPluginState threw, the api was already in the WeakSet, permanently blocking any retry from the same api instance. - Fix: wrap _initPluginState in try-catch; add api to WeakSet only after successful init. If init fails, api stays out of WeakSet, allowing subsequent register() calls with the same api to retry. Fixes CortexReach#448
There was a problem hiding this comment.
💡 Codex Review
memory-lancedb-pro/src/reflection-store.ts
Lines 441 to 444 in 987fc83
if for derived ownership branch
The if (typeof itemKind === "string" && itemKind === "derived") token sequence is now inside a // comment, so the following if (!owner) return false;/return owner === agentId; block is no longer guarded and leaves a stray closing brace. In this commit state, src/reflection-store.ts fails to parse (TS1128: Declaration or statement expected), which blocks TypeScript-based builds and any runtime path that loads this module.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Summary
Fixes a P1 registration lifecycle regression in
register()where_registeredApis.add(api)was called before_initPluginState(). If_initPluginStatethrew, the api was already in the WeakSet, permanently blocking any retry from the same api instance.Fix
Wrap
_initPluginStatein a try-catch; add api to WeakSet only after successful init. If init fails, the api stays out of the WeakSet, allowing subsequentregister()calls with the same api to retry.Relationship to PR #522
This PR supersedes the
fix/issue-448-v2branch. PR #522 addressed theisOwnedByAgentderived ownership fix but had an outstanding Must-Fix #1 (registration lifecycle regression). This PR fixes that regression.index.ts_registeredApis.add()moved to after successful_initPluginState()with try-catchTesting
src/reflection-store.tsunrelated to this change)