Fix flakiness of prolog subscription queries#589
Conversation
WalkthroughThe pull request modifies the subscription initialization and handling across two modules. In the core module, a new private promise member Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant PP as PerspectiveProxy
participant QSP as QuerySubscriptionProxy
C->>PP: subscribeInfer(query)
PP->>QSP: Instantiate & initiate async subscription
QSP->>QSP: Wait for first result (resolve #initialized)
QSP-->>PP: Promise resolved
PP-->>C: Return subscription proxy
sequenceDiagram
participant C as Client
participant PI as PerspectiveInstance
participant Task as Async Task
participant PubSub as Global PubSub
C->>PI: subscribe_and_query(query)
PI->>PI: Insert subscription in map
PI->>Task: Spawn async task with 100ms delay
Task->>PubSub: Publish initial result (JSON format)
Task-->>PI: Log the publishing action
PI-->>C: Return subscription details
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (9)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/src/perspectives/PerspectiveProxy.ts (1)
73-84: Improved subscription initialization with Promise patternThis change encapsulates the subscription logic within a Promise that resolves when the first result is received, ensuring proper subscription initialization.
However, there's no error handling if the subscription setup fails.
Consider adding error handling by adding a timeout or rejection mechanism:
this.#initialized = new Promise<boolean>((resolve, reject) => { // Add timeout to prevent hanging promises const timeoutId = setTimeout(() => { reject(new Error('Subscription initialization timed out')); }, 30000); // 30 seconds timeout // Subscribe to query updates this.#unsubscribe = this.#client.subscribeToQueryUpdates( this.#subscriptionId, (result) => { clearTimeout(timeoutId); resolve(true); this.#latestResult = result; this.#notifyCallbacks(result); } ); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/src/perspectives/PerspectiveProxy.ts(4 hunks)rust-executor/src/perspectives/perspective_instance.rs(1 hunks)rust-executor/src/pubsub.rs(2 hunks)tests/js/bootstrapSeed.json(1 hunks)tests/js/publishBootstrapSeed.json(1 hunks)tests/js/tests/prolog-and-literals.test.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- rust-executor/src/pubsub.rs
- tests/js/publishBootstrapSeed.json
🧰 Additional context used
🧬 Code Definitions (1)
tests/js/tests/prolog-and-literals.test.ts (2)
core/src/model/Ad4mModel.ts (1)
perspective(336-338)core/src/perspectives/PerspectiveResolver.ts (1)
perspective(66-68)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Fleek - docsad4m
🔇 Additional comments (6)
rust-executor/src/perspectives/perspective_instance.rs (1)
2044-2064: Well-implemented improvement to subscription initializationThis change adds a 100ms delay before sending the initial subscription result, which helps synchronize query subscriptions and reduce flakiness as mentioned in the PR objectives. The implementation correctly:
- Clones necessary variables to avoid lifetime issues in the spawned task
- Uses a controlled delay with
sleepto improve timing- Creates and publishes the subscription filter properly
tests/js/bootstrapSeed.json (1)
1-1: Configuration updated with new keysThis update changes several values in the configuration file including:
- trustedAgents
- knownLinkLanguages
- directMessageLanguage
- agentLanguage
These changes appear to be part of the overall update to address the flakiness issues by ensuring consistent configuration across the codebase.
core/src/perspectives/PerspectiveProxy.ts (3)
59-59: New private field for tracking subscription initializationAdding a Promise to track when the subscription is fully initialized is a good approach to help resolve the flakiness issues mentioned in the PR description.
108-110: New getter for accessing initialization statusThis getter provides a clean way to access the initialization Promise, making it available for outside components like the
subscribeInfermethod to wait on.
1087-1101: Fixed race condition by waiting for initializationThis change is the key fix for the flakiness issue. By waiting for the
initializedPromise to resolve before returning the subscription proxy, you ensure that the subscription is fully established and the initial result is available.The approach aligns well with the Rust-side changes mentioned in the PR description, which add a 100ms delay before sending the initial result.
tests/js/tests/prolog-and-literals.test.ts (1)
1761-1774: Enhanced subscription waiting mechanism to reduce flakiness.This modification improves the reliability of waiting for subscription updates by:
- Switching from a while loop to a more structured for loop
- Increasing the number of attempts from implicit 20 to explicit 50
- Reducing sleep interval from 500ms to 100ms (aligning with the 100ms delay added in the Rust code)
- Adding logging to provide better visibility during test execution
- Providing a more informative timeout error message
The total timeout period is now 5 seconds (down from 10), but with more frequent checking, which should work better with the changes made to the subscription initialization process.
1 participant
Fix Flaky Behavior in Prolog Query Subscriptions
This PR addresses flaky behavior in the Prolog query subscription system by improving the synchronization between subscription setup and initial result delivery.
Key Changes
Rust Side (
perspective_instance.rs)JavaScript Side (
PerspectiveProxy.ts)awaitinitial result in client after subscription setup to ensure the subscription is fully set up before we continueQuerySubscriptionProxyto properly handle initializationinitializedPromise that resolves when the first subscription result is receivedsubscribeInferto wait for the subscription to be fully initialized before returningSummary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation