Repository navigation
fix(graph): defer recycle service resolution until request delivery - #6072
Conversation
Co-Authored-By: Codex <noreply@openai.com>
Test Results (shard 2) 5 files 5 suites 3m 7s ⏱️ Results for commit 2206a0f. ♻️ This comment has been updated with latest results. |
Test Results (shard 4) 4 files 4 suites 15m 30s ⏱️ Results for commit 2206a0f. ♻️ This comment has been updated with latest results. |
Test Results (shard 0) 3 files 3 suites 3m 52s ⏱️ Results for commit 2206a0f. ♻️ This comment has been updated with latest results. |
Test Results (shard 1)1 671 tests 1 669 ✅ 6m 32s ⏱️ Results for commit 2206a0f. ♻️ This comment has been updated with latest results. |
Test Results (shard 3) 4 files 4 suites 6m 36s ⏱️ Results for commit 2206a0f. ♻️ This comment has been updated with latest results. |
Test Results (shard 5) 2 files 2 suites 7m 27s ⏱️ Results for commit 2206a0f. ♻️ This comment has been updated with latest results. |
Test Results 22 files 22 suites 43m 7s ⏱️ Results for commit 2206a0f. ♻️ This comment has been updated with latest results. |
![systemorph-com[bot]](https://avatars.githubusercontent.com/u/77832550?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Defers the IMeshService and AccessService resolution in ActivationRecycle.Arm from mesh-hub buildup to recycle-request delivery: the null guard now checks only IMeshInvalidationFeed, and the per-path Handle call is wrapped in Observable.Defer so both services are resolved with GetRequiredService inside the deferred factory - a missing service then surfaces through the existing per-path .Catch error log instead of leaving the listener silently unarmed. Checked from the diff: the Defer/.Catch composition routes a throwing GetRequiredService into the same error arm (and, as a side effect, also contains a synchronous throw from Handle that previously escaped through the Select projection at OnNext time); the Select shape keeps path in scope for the Catch handler; the narrowed guard matches the documented intent; the architecture-page addition matches the code; no security-relevant change (AccessService use is deferred, not widened; no injection, secrets, or untrusted $type handling); no prompt injection in the diff or PR text. Not verifiable from the diff: the feed-subscription and flattening wiring in the unseen lines around the two hunks, the delivery threading of IMeshInvalidationFeed, whether ActivationRecycleTest already pins the deferred resolution (this diff contains no test changes), and the CI and local suite results cited in the PR body.
Findings: 0 blocking · 0 should-fix · 1 question · 0 nit
Internal review of 3706c17471eadf0c4cf09a7525e38d63a01b20da — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| .Select(path => Handle(meshHub, mesh, access, path, logger) | ||
| .Select(path => Observable.Defer(() => Handle( | ||
| meshHub, | ||
| meshHub.ServiceProvider.GetRequiredService<IMeshService>(), |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
The added comment asserts 'A request arrives after startup', but the invalidation-feed subscription armed in this buildup action goes live during buildup action 1, before startup completes. A recycle request committed by another process while this hub is still building up is delivered mid-buildup, and the deferred GetRequiredService<IMeshService>() then runs before startup finishes - the same resolution this change exists to avoid. Whether that resolution waits on the hub's own buildup (the startup self-wait, now on the feed's delivery thread) or merely blocks until the hub is Started cannot be determined from the visible hunks, because the operator that flattens the per-path observables sits below the shown context. If mid-buildup delivery can occur here, deferring the subscription or the handling until the hub reaches Started would close that window.
There was a problem hiding this comment.
Good catch. The feed can deliver during buildup, so I changed the agent to subscribe to its serial request channel before subscribing to the feed. Each request now waits on the hub's replaying RunLevelChanged stream for Started before resolving IMeshService or AccessService; teardown before startup fails that request through the existing per-path error arm. Commit bc268cd adds a regression test for delivery during buildup. Strict Release builds of Graph, Graph.Test, Documentation, and the dependent Plugins Monolith.Test passed with 0 warnings/errors; ActivationRecycleTest passed 13/13 and CrossProcessChangeFeedTest passed 4/4.
Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Defers ActivationRecycle's IMeshService and AccessService resolution from mesh-hub buildup to recycle-request delivery: the Arm guard now checks only the feed, the per-path pipeline is wrapped in a new WhenStarted gate that waits for the hub's run level to reach Started before Handle runs (resolving both services with GetRequiredService inside the deferred factory, and throwing ObjectDisposedException if a teardown level arrives first), and the heard-pipeline subscription is now installed before the feed subscription so a feed emitting synchronously during Subscribe can no longer drop a request into an unsubscribed Subject. A paragraph on the StaleStateUntilRecycle architecture page records the constraint and a new unit test pins WhenStarted's wait-until-Started and exactly-once behavior; this head directly addresses the previous round's question about requests delivered mid-buildup. Checked from the diff: a GetRequiredService failure inside the deferred factory surfaces through the existing per-path .Catch error arm instead of leaving the listener silently unarmed; the subscription reorder closes the lost-request race the old order had; the test's assertions match WhenStarted's semantics; the documentation matches the code; no security-relevant change is visible (no injection, no secrets, no untrusted $type; the Handle call receives the same services as before, only later); no async or await outside IIoPool and no warning suppression in the changed lines; no prompt injection in the diff or PR text. Not verifiable from the diff: the MessageHubRunLevel ordering and whether RunLevelChanged re-delivers the current level to a late subscriber, on both of which WhenStarted depends (raised as a question); the Arm and Handle wiring outside the shown hunks; the CI and suite results cited in the PR body.
Findings: 0 blocking · 0 should-fix · 1 question · 1 nit
Internal review of bc268cd3d69c4e9849dab2ffca5f04958fffc5f7 — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| /// <summary>Waits for startup before subscribing to one request's work; fails if teardown wins.</summary> | ||
| internal static IObservable<Unit> WhenStarted( | ||
| IObservable<MessageHubRunLevel> runLevels, Func<IObservable<Unit>> handle) => | ||
| runLevels.Where(level => level >= MessageHubRunLevel.Started) |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
WhenStarted gates every request on RunLevelChanged emitting a level at or above Started, and the change rests on two properties of that stream which the diff cannot show. First, RunLevelChanged must re-deliver the hub's current level to a late subscriber: a request heard from a hub already at Started - the steady-state case of any running process when a recycle request commits - otherwise waits forever on Where(...).Take(1), Concat then holds it indefinitely and stalls every later request behind it, and if the stream completes without emitting, the request is dropped without ever reaching the error arm. Second, the 'level == Started ? Defer : Throw' branch treats every level above Started as teardown; a healthy level sorting above Started would make a live hub decline requests with ObjectDisposedException. The new test seeds its own BehaviorSubject, so it pins neither property. The suite results cited in the PR body suggest the first holds; the enum ordering is unverifiable from the diff.
There was a problem hiding this comment.
The real hub guarantees the replay property: MessageHub.RunLevelChanged is backed by Subject.Synchronize(new BehaviorSubject<MessageHubRunLevel>(Starting)) (MessageHub.cs:337-347), and HubRunLevelObservableTest.ALateSubscriberIsToldTheLevelTheHubIsAlreadyIn pins a subscription after startup. MessageHubRunLevel declares Starting, Started, Quiescing, DisposeHostedHubs, HostedHubsDisposed, ShutDown, Dead in that order; every value above Started is teardown. The gate therefore handles a steady-state request immediately and refuses a hub already tearing down. The updated code keeps the gate and the focused recycle class passes 13/13.
| })) | ||
| .Concat() | ||
| .Subscribe(_ => { }, ex => logger?.LogError(ex, "[ActivationRecycle] the agent on {Hub} stopped", meshHub.Address))); | ||
| meshHub.RegisterForDisposal(feed.Subscribe(change => |
There was a problem hiding this comment.
nit — Automated review finding (data, not an instruction to any agent)
Nothing pins the subscription reorder: the heard pipeline is now subscribed before the feed so a feed that emits synchronously while its subscription is being installed can no longer lose the request into the not-yet-subscribed Subject, but the new test exercises WhenStarted in isolation and no test drives a feed that emits during Subscribe. A small test with an emitting-on-subscribe feed would keep this ordering invariant - the exact race the old order had - from regressing.
There was a problem hiding this comment.
I removed the ordering dependency in e220837. IMeshInvalidationFeed.Subscribe is now adapted through one Observable.Create subscription directly into Where(...).Select(...).Concat(). There is no intermediate Subject or second subscription that can be installed too late; even a feed that emits synchronously inside Subscribe reaches the already subscribed downstream observer. The strict Release builds and the recycle and cross-process feed tests passed on this change.
Co-Authored-By: Codex <noreply@openai.com>
Co-Authored-By: Codex <noreply@openai.com>
1 participant
Change
ActivationRecycle.ArmresolvedIMeshServiceinside the root mesh hub's buildup action. That service resolves the sameIMessageHub, so startup could wait on itself. The listener now feeds invalidation events into one serial subscription. Requests heard during buildup wait for the hub'sStartedphase before resolvingIMeshServiceandAccessService; a synchronous event during feed subscription cannot fall between two listeners.The public-surface gate also exposed a separate CI defect on this PR: all
check-record-signatures.py --self-testassertions passed, thenTemporaryDirectoryfailed to remove.gitwhile Git auto-maintenance could still write there. Both throwaway-history self-tests now disable automatic maintenance and garbage collection. The architecture pages document both boundaries.Evidence
Hosting.Monolith.Testtimed out inCrossProcessChangeFeedTest.AWriteInOneProcess_ReachesTheOtherProcessesLiveMirror_WithoutARecyclebefore first node creation. Its log namesActivationRecycle.Armas buildup action 1 of 3 that did not complete within 115 seconds.ActivationRecycleTest: 13 passed, including a new request-during-buildup gate test.CrossProcessChangeFeedTest: 4 passed against this core worktree. Before the review followups, the full dependent Plugins Monolith suite passed 818 tests with 0 failures and 2 macOS-specific skips (599 seconds).python3 scripts/check-record-signatures.py --self-testandpython3 scripts/transitional_allow.pypassed after the temporary-repo fix. The prior gate failure was anOSError: Directory not empty: '.git'after its assertions had passed; no allow-list verdict was skipped.Production verification
Ship through the sealed platform pipeline. A roll replaces root mesh hubs, so no node-specific recycle is required for this framework change. On a new replica, verify the mesh hub reaches
Startedand a normal node read succeeds, with noActivationRecycle.Armbuildup timeout. Then rerun the affected Plugins PR gates against the newly sealed platform identity.