fix(core): retry domain subscriber registration after bus startup - #5400
fix(core): retry domain subscriber registration after bus startup#5400samrusani wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughDomain and learning subscriber registration now checks global event-bus readiness before consuming completion tokens. Unavailable-bus attempts remain retryable. Successful registration remains idempotent. Tests cover both flows and the domain wrapper. ChangesDomain subscriber registration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Registration as register_domain_subscribers
participant Helpers as registration helpers
participant Bus as GlobalEventBus
participant Completion as Per-process completion tracking
Registration->>Helpers: Attempt subscriber registration
Helpers->>Bus: Check event-bus readiness
alt Bus unavailable
Helpers-->>Registration: Defer without consuming token
else Bus available
Helpers->>Completion: Record group completion
Helpers->>Bus: Register subscribers
end
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
coderabbitai
Bot
added
the
rust-core
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/jsonrpc.rs`:
- Around line 1944-1972: Split src/core/jsonrpc.rs into focused Rust modules so
it is at most 500 lines, moving the bus-aware registration tracking around
group_first_time_when_bus_ready and group_first_time together with the
domain-subscriber registration logic into an appropriate module. Update imports,
visibility, and call sites to preserve existing retry behavior when the event
bus is unavailable and registration remains one-time after initialization.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cac5fb14-6b11-4a9b-9975-88abd5f27a86
📒 Files selected for processing (2)
src/core/jsonrpc.rssrc/core/jsonrpc_tests.rs
|
| Filename | Overview |
|---|---|
| src/core/jsonrpc.rs | Refactors group_first_time and learning_first_time to gate completion-set insertion on bus readiness; removes the now-resolved known-limitation comment; logic is correct and well-documented. |
| src/core/jsonrpc_tests.rs | Adds five focused unit tests covering deferred, retry, idempotency, and global-bus wiring paths; tests are clear and directly map to the new behaviour. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[register_domain_subscribers called] --> B{bus_ready?}
B -- No --> C[log warn: deferred registration\nreturn false]
C --> D[Group NOT inserted into DONE\nRemains retryable]
B -- Yes --> E{Group already in DONE?}
E -- Yes --> F[return false\nIdempotent skip]
E -- No --> G[Insert group into DONE\nreturn true]
G --> H[Run subscriber registration block]
H --> I[SubscriptionHandle forgotten\nLive for process lifetime]
D --> J[Next bootstrap call retries]
J --> B
Reviews (2): Last reviewed commit: "fix(core): keep learning subscriber regi..." | Re-trigger Greptile
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e008bc24f
ℹ️ 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".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/jsonrpc_tests.rs (1)
184-192: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake the production-wrapper test independent of global mutex state.
group_first_timeuses process-globalgroup_first_time_when_bus_ready(...completed...)state. This wrapper test assertsgroup_first_time(DomainGroup::Media)returnstrue, but any preceding test that enables theMediagroup can consume the token and make this test fail under parallel execution. Use the isolatedgroup_first_time_when_bus_readyfixture with an explicit bucket, or add mutex reset/exposure in the test helper for this production wrapper path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/jsonrpc_tests.rs` around lines 184 - 192, Update domain_subscriber_registration_wrapper_uses_the_global_bus to avoid relying on shared group_first_time_when_bus_ready completed-state: use the isolated fixture with an explicit bucket, or reset/expose the test mutex state before exercising group_first_time(DomainGroup::Media). Preserve the assertions that the first registration succeeds and the second returns false.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/core/jsonrpc_tests.rs`:
- Around line 184-192: Update
domain_subscriber_registration_wrapper_uses_the_global_bus to avoid relying on
shared group_first_time_when_bus_ready completed-state: use the isolated fixture
with an explicit bucket, or reset/expose the test mutex state before exercising
group_first_time(DomainGroup::Media). Preserve the assertions that the first
registration succeeds and the second returns false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 38528ce4-0d45-4ede-abe1-a2a0d5546db0
📒 Files selected for processing (2)
src/core/jsonrpc.rssrc/core/jsonrpc_tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/jsonrpc.rs
|
Reviewed the outside-diff suggestion about the |
|
Maintainer review (merge-readiness sweep) — review only, I have not touched this branch. The change itself is sound: making the group token conditional on bus readiness is the right shape for #5265, the pure However, it will not compile on current What broke
What it should becomeThe readiness predicate is now - crate::core::event_bus::global().is_some(),
+ crate::core::bus::BUS.get().is_some(),in both The test needs more than a rename: #[tokio::test]
async fn domain_subscriber_registration_wrapper_uses_the_global_bus() {
use crate::core::all::DomainGroup;
// The global bus has to be up for the wrapper to consume its token at all;
// `init` is idempotent, so it does not matter whether an earlier test in
// this binary already stood it up.
let _ = crate::core::bus::init().await;
assert!(group_first_time(DomainGroup::Media));
assert!(!group_first_time(DomainGroup::Media));
}
I applied exactly those three edits locally on top of a One thing worth a second look
To landMerge or rebase onto |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
How this change flows0 changed behaviours across 8 relationships. 4 surrounding behaviours are shown (60 graph nodes walked). 41 further behaviours left out to keep the diagram readable. flowchart LR
n0["vec"]:::impacted
n1["set_many"]:::impacted
n2["...est_payload_is_captured_at_warn_not_error"]:::impacted
n3["..._envelope_passes_through_generic_dispatch"]:::impacted
n2 -->|calls| n0
n2 -->|tests| n0
n2 -->|calls| n1
n2 -->|tests| n1
n3 -->|calls| n0
n3 -->|tests| n0
n3 -->|calls| n1
n3 -->|tests| n1
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
tinysweeper
Bot
added
the
priority: p3
|
I rebased and rechecked the requested current-main path before publishing. On current main, bootstrap_core_runtime awaits core::bus::init() immediately before the sole production call to register_domain_subscribers; start_channels also awaits bus initialization and no longer calls that registrar. I therefore did not push the defensive rebase, since the remaining trigger described in the PR no longer reproduces. Is there another current path you want this PR to cover, or should I close it as obsolete? |
2 participants
Summary
Problem
group_first_timeinserted aDomainGroupinto the process-wide completed set before its registration block ran. If the block ran before the global event bus was initialized, everysubscribe_globalcall returnedNone, but the group stayed marked complete and could never retry during the process lifetime.Solution
Gate insertion into the completed set on global event-bus readiness.
subscribe_globalhas only one failure condition: the monotonic global busOnceLockhas not been initialized. Once the bus exists it cannot disappear, so the existing registration blocks can run unchanged and remain idempotent.The state transition is extracted behind an injectable readiness flag for deterministic failure-path coverage. A separate wrapper test exercises the real global-bus lookup with a domain token that has no subscriber block.
On current main,
bootstrap_core_runtimeinitializes the bus before it registers domain subscribers, which narrows the normal-startup window.start_channelscan still reach registration before that path; retaining the readiness gate keeps a premature attempt retryable rather than permanently consuming its token.Submission Checklist
diff-coveron the current branch head.## Related- N/A: no matrix feature ID applies.Closes #NNNin the## Relatedsection.Impact
Core runtime only. A premature registration attempt now defers cleanly and can succeed on a later bootstrap call. Current bootstrap ordering narrows the normal-startup window, but
start_channelscan still arrive before it; normal startup, domain widening, and once-per-process behavior are otherwise unchanged. No persistence, migration, API, or user-interface impact.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
agent/retry-domain-subscriber-registration8e4eed191ff069843f40fdb938d4a095d03fbb3dValidation Run
pnpm --filter openhuman-app format:check- N/A: no frontend files changed.pnpm typecheck- N/A: no TypeScript files changed.cargo test --lib domain_subscriber -- --nocapture(6 passed); repository scoped coverage lane (97 passed, 1 pre-existing ignored);diff-cover(100%).cargo fmt --manifest-path Cargo.toml --check;git diff --check; default and slim-feature focused test builds passed.Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
--no-default-features --features tokenjuice-treesitterregistrar tests both pass.Duplicate / Superseded PR Handling
Summary by CodeRabbit