Skip to content

feat: dual-track registration with early claim + rollback support#721

Open
jlin53882 wants to merge 2 commits intoCortexReach:masterfrom
jlin53882:fix/issue-448-dual-track
Open

feat: dual-track registration with early claim + rollback support#721
jlin53882 wants to merge 2 commits intoCortexReach:masterfrom
jlin53882:fix/issue-448-dual-track

Conversation

@jlin53882
Copy link
Copy Markdown
Contributor

Summary

Dual-track registration system for memory-lancedb-pro:

  • Before init (Phase 2): claim api in both (WeakSet) and (Map)
  • On init failure: explicit rollback, enabling retry with same API object
  • resetRegistration(): now clears both WeakSet and Map

This enables test inspection of registered APIs and provides explicit rollback semantics on init failure.

Changes

  • : +30/-6 lines (single file)

Dependencies

Based on master after PR #522 merge.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 013c1785b3

ℹ️ 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".

Comment thread index.ts Outdated
singleton = _singletonState;
} catch (err) {
api.logger.error(`memory-lancedb-pro: _initPluginState failed — ${String(err)}`);
_registeredApisMap.delete(api); // dual-track rollback: init failed, un-claim
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Roll back WeakSet claim when init fails

If _initPluginState(api) throws, the catch block only removes api from _registeredApisMap, but the earlier _registeredApis.add(api) is never undone. Because register() gates on _registeredApis.has(api), a later retry with the same API object is skipped permanently, so initialization cannot recover after a transient startup failure. Add _registeredApis.delete(api) in this failure path (or delay the WeakSet add until after successful init) to preserve retry behavior.

Useful? React with 👍 / 👎.

@jlin53882
Copy link
Copy Markdown
Contributor Author

Why This PR Matters

The Problem

After PR #522, places only after successful . This means:

  • On init failure: the WeakSet has zero record that a registration was ever attempted — no trace, no debug trail
  • Retry logic has no foundation: a subsequent call with the same API has no way to know "this was already tried and failed"
  • Tests cannot assert: there is no way to verify "was this API object already considered for registration?" — WeakSet only tells you "is currently registered", not "was attempted"

The Dual-Track Solution

Track Data Structure Purpose
Phase 2 singleton guard — GC-safe, prevents double-init
Explicit claim/rollback — inspectable, supports retry logic

Registration flow now:

Concrete Benefits

  1. Testability — lets tests assert "this API was attempted" even after a failed init
  2. Debuggability — explicit claim/rollback creates a traceable audit trail
  3. Retry safety — provides the ground truth for "should this API be retried?"
  4. Future extensibility — the Map can carry metadata (timestamp, retry count, error reason) that a WeakSet structurally cannot

Codex PR CortexReach#721 review: if _initPluginState throws, only _registeredApisMap
was rolled back, but _registeredApis.add(api) happened BEFORE the try block
so the WeakSet entry remained. This caused register()'s _registeredApis.has(api)
guard to permanently skip retry — plugin could not recover from transient
init failures.

Fix: delete from both WeakSet and Map in the catch block.
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.

1 participant