oor: add unified OOR service for outgoing + incoming orchestration - #118
Conversation
Summary of ChangesHello @bhandras, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the OOR (Outgoing/Incoming Orchestration and Resolution) system by introducing a unified Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new OORService to abstract away the complexities of outgoing and incoming OOR orchestration. The new service provides a clean, high-level API, and the implementation is well-structured with comprehensive tests. The changes significantly improve the usability and maintainability of the OOR features. I have a couple of suggestions regarding context handling and the choice of random number generator for performance and robustness.
| return nil | ||
| } | ||
|
|
||
| loopCtx, cancel := context.WithCancel(context.Background()) |
There was a problem hiding this comment.
The worker's context is created from context.Background(), which detaches its lifecycle from the ctx passed to StartIncomingSync (which is ignored on line 178). This can be misleading and may lead to the worker not being stopped when the caller's context is canceled. To ensure proper lifecycle management and prevent potential resource leaks, the worker's context should be derived from the parent context passed to the function. You will also need to remove the _ = ctx on line 178.
| loopCtx, cancel := context.WithCancel(context.Background()) | |
| loopCtx, cancel := context.WithCancel(ctx) |
There was a problem hiding this comment.
Good catch, fixed.
StartIncomingSync now derives the worker context from the provided ctx (context.WithCancel(ctx)) instead of context.Background(), with a nil-context guard that falls back to context.Background().
This ties worker lifetime to caller cancellation while preserving explicit StopIncomingSync shutdown behavior.
| jitterMax := big.NewInt(int64(s.incomingPollJitter) + 1) | ||
| noise, err := cryptorand.Int(cryptorand.Reader, jitterMax) | ||
| if err != nil { | ||
| return delay | ||
| } | ||
|
|
||
| jitter := time.Duration(noise.Int64()) |
There was a problem hiding this comment.
Using crypto/rand for generating jitter in a polling delay is unnecessary and can have performance implications, as it can block if the system is low on entropy. For non-security-critical randomness like this, math/rand is more appropriate and performant.
To implement this, you can add a rand *rand.Rand field to the oorService struct, initialize it in NewOORService with rand.New(rand.NewSource(time.Now().UnixNano())), and then use it here to generate the jitter.
| jitterMax := big.NewInt(int64(s.incomingPollJitter) + 1) | |
| noise, err := cryptorand.Int(cryptorand.Reader, jitterMax) | |
| if err != nil { | |
| return delay | |
| } | |
| jitter := time.Duration(noise.Int64()) | |
| jitterVal := s.rand.Int63n(int64(s.incomingPollJitter) + 1) | |
| jitter := time.Duration(jitterVal) |
There was a problem hiding this comment.
Applied.
The jitter path now uses a non-crypto PRNG (math/rand) stored on the service struct (jitterRand) instead of crypto/rand.
This keeps the polling delay jitter fast and avoids blocking entropy reads for non-security randomness.
126bb02 to
3ee98c4
Compare
| return SessionID{}, fmt.Errorf("service must be provided") | ||
| } | ||
|
|
||
| result := s.actor.Receive(ctx, &StartTransferRequest{ |
There was a problem hiding this comment.
Why direct recv on actor vs adding it to the system and using a service key to get a normal ref?
There was a problem hiding this comment.
I took a closer look at the serverconn/mailbox plumbing direction in PR #116, and I think your point stands: if we route incoming through that connector path, the incoming orchestration I added here is likely unnecessary.
Given that direction, this PR should probably be treated as temporary scaffolding (or closed) rather than the long-term shape.
| } | ||
|
|
||
| // GetOutgoingState returns a caller-facing state summary for one session. | ||
| func (s *oorService) GetOutgoingState(ctx context.Context, |
There was a problem hiding this comment.
So then what ends up driving this? Thought we had a like coordinator actor that would talk to the vtxo actors when things need to happen based on the wallet or server?
There was a problem hiding this comment.
Agreed. After looking at PR #116 more closely, the intended model is to drive incoming server events through the unified connector/mailbox path into actors, not through a service-local polling loop.
So the incoming orchestration in this PR is likely redundant with that target architecture.
| } | ||
| defer session.FSM.Stop() | ||
|
|
||
| outbox, err := askFSMEvent(ctx, session.FSM, &IncomingTransferEvent{ |
There was a problem hiding this comment.
Same here. Does this need to be a durable actor? Then can makes sure stuff like send/recv OOR is properly restarted.
There was a problem hiding this comment.
Yes — after reviewing PR #116, I agree the long-term durable restart story should come from mailbox/serverconn ingress + actor dispatch, not this service-level incoming loop.
Given that, the incoming receive path in this PR is probably not worth carrying forward as-is. We can close this and reintroduce only the pieces that still fit once the connector plumbing lands.
3ee98c4 to
de8b95e
Compare
715a781 to
1c40b23
Compare
Define a high-level OORService API surface for outgoing orchestration, incoming sync, and unroll package resolution. The new contracts include incoming event-source and cursor-store abstractions so runtime code can stay decoupled from specific transport and persistence implementations. Add a DBIncomingCursorStore adapter that maps OOR artifact store script and cursor rows into the service interfaces.
Build the concrete OOR service implementation on top of the durable outgoing actor and the receive-side FSM. The service now runs incoming script polling, validates cursor/event ordering, drives materialize+ack flows, and only advances cursors after successful processing. The implementation also adds lifecycle controls for background incoming sync workers and exposes typed outgoing state and unroll resolver facades.
Add service-level tests for outgoing happy-path orchestration, incoming sync cursor progression, ack-failure cursor safety, resolver passthrough, and worker start/stop behavior. These tests lock in the key split-6 guarantees: incoming cursors only move after materialize+ack succeeds and callers can drive both outgoing and incoming behavior through one high-level API.
de8b95e to
49e1ff2
Compare
|
Closing this as the direction is already set in #116 and therefore this won't be necessary or not in this current form. We can always revisit once we're in the plumbing stage. |
rounds: purify FSM transitions via outbox handler pattern
2 participants
Summary
This split introduces a high-level
OORServiceAPI on top of existing OORprimitives so callers no longer manually orchestrate:
The service provides one cohesive surface for:
Base:
oor-client-split-5-unroll-packages-draft.Architecture Alignment
This PR now supports the same actor-system wiring pattern used by VTXO/round:
AsktoActorRefinstead of hard-wiring direct calls,Concretely:
ActorServiceKey(actorID)inoor/service_key.go,ServiceConfig.ActorSystem,ServiceConfig.OutgoingServiceKey, andServiceConfig.OutgoingRef,NewOORServiceresolves outgoing in this order:OutgoingRef(explicit injection),NewOORClientActorfallback (requiresDeliveryStore).This keeps integration flexible while matching the system-wide actor plumbing
model when actor system wiring is available.
Problem This Split Solves
By split 5, OOR primitives were correct, but app/harness integration still
required low-level choreography in multiple places:
This split centralizes that orchestration in one tested runtime service.
What Was Added
1) Service contracts and adapters
oor/service_types.goOORServiceinterfaceIncomingEventSource,IncomingCursorStore,UnrollPackageResolveroor/service_db_adapters.goDBIncomingCursorStoreadapter fromdb.OORArtifactPersistenceStoretoIncomingCursorStoreoor/service_key.go2) Service runtime
oor/service.goNewOORService(cfg ServiceConfig)constructorStartOutgoingGetOutgoingStateSyncIncomingOnceStartIncomingSyncStopIncomingSyncGetIncomingSyncStatusResolveUnrollPackagesStop3) Service-level tests
oor/service_test.goRuntime Architecture
Outgoing path
OORService.StartOutgoing-> configured outgoing actor endpoint:OutgoingRef, orServiceKey.Ref(...), orOORClientActorfallback.Service callers use
GetOutgoingStatefor typed state view.Incoming path
SyncIncomingOnce(or worker cycle):afterEventID,IncomingTransferEvent),ReceiveCompleted,event_id.Flow:
script -> cursor -> events -> validate -> materialize -> ack -> cursor++Cursor safety invariant:
Scope Notes
IncomingEventSource), so a futurelong-poll or stream source can be injected without changing service API.
Testing
Executed locally:
go test ./...make lintmake commitmsg-lint range=origin/main..HEADAll green.