fix(observatory): poll no longer reverts an optimistic steer value mid-write#1423
Conversation
…flight Fold the deferred gitar Edge-Case from #1420: the 5s fleet/throttle poll could land mid-request and revert an optimistic steer value (pause, global cap, or lane cap) before the POST completed. Track in-flight steer writes with a ref incremented in the shared postSteer; the interval skips a tick while any write is pending, and postSteer's existing reconcile updates authoritatively once the write lands. Uniform across all three controls. New test asserts the poll is skipped during an in-flight write and resumes after it resolves.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesPoll gating during steer writes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| const steerSeq = useRef(0); | ||
| const postSteer = useCallback( | ||
| async (url: string, body: object, failMsg: string) => { | ||
| inFlight.current += 1; |
There was a problem hiding this comment.
SUGGESTION: inFlight.current += 1 runs before the try block, so the matching decrement in finally will not execute if anything between this line and try { ever throws. Today the only statement in between (const seq = ++steerSeq.current;) cannot throw, so it is safe in practice, but the ordering is fragile — a future edit (e.g., adding input validation) could leave inFlight permanently positive and silently disable the background poll for the lifetime of the component. Move the increment inside the try, or wrap the full body in a top-level try { ... } finally { inFlight.current -= 1; }.
| const id = setInterval(() => { | ||
| // Skip the poll while a steer write is in flight so it cannot revert an | ||
| // optimistic value mid-request; postSteer reconciles after the write. | ||
| if (inFlight.current === 0) load({ silent: true }); |
There was a problem hiding this comment.
SUGGESTION: When the 5s tick fires while inFlight > 0, the tick is silently dropped and the next fire is still ~5s away. If a steer write (or a backlog of overlapping writes) keeps inFlight non-zero for longer than the interval, the poll effectively halts; the only state refresh becomes the per-write reconcile load() in finally. That is acceptable for short writes, but a stuck or long-lived POST would let fleet/throttle state drift. Consider rescheduling a tick immediately when inFlight returns to 0, or shortening the interval's drift budget when a tick is skipped.
| render(<ObservatoryApp windowId="w1" />); | ||
| await act(async () => { await Promise.resolve(); }); | ||
|
|
||
| // Start a pause write whose POST stays pending so inFlight stays non-zero. |
There was a problem hiding this comment.
SUGGESTION: The PR description claims uniformity across setScope / setGlobalCap / setLaneCap, but this new case only exercises the setScope (pause) path — the global cap stepper and per-lane cap stepper are not asserted to skip-then-recover under the same conditions. Since all three route through the same postSteer, one path is arguably enough for the inFlight mechanism itself, but the symmetry claim in the description is not backed by these tests. Consider mirroring the assertion against Raise concurrency cap and Raise @taOS-dev-kilo-owl-alpha limit (with their POSTs held pending).
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| } finally { | ||
| await load({ silent: true }); | ||
| inFlight.current -= 1; | ||
| } |
There was a problem hiding this comment.
💡 Edge Case: Reconcile load() can still revert a concurrent in-flight steer write
The new inFlight guard only suppresses the 5s background poll. However, postSteer's finally block unconditionally calls await load({ silent: true }) regardless of whether other steer writes are still pending. Because the controls are intentionally not serialized (they use different busy scopes — global, cap, cap:<handle> — per the comment at lines 140-145), two writes can overlap: e.g. a lane-cap write A is still in flight (optimistic value set) while a pause write B resolves first. B's reconcile load() fetches authoritative state from the server, which does not yet reflect A's not-yet-completed POST, so A's optimistic value is briefly reverted — exactly the class of flicker this PR aims to eliminate, now triggered by a sibling write rather than the interval.
This is a narrow case (requires rapid operation of two different controls), hence minor, but it undercuts the PR's "uniform across setScope/setGlobalCap/setLaneCap" claim. Consider only running the reconcile load() when it is the last write to settle (e.g. skip the reconcile fetch while inFlight.current > 1, letting the final write reconcile), or gating the reconcile on seq === steerSeq.current.
Skip the reconcile fetch while other steer writes are still in flight; the final write performs the authoritative reconcile.:
} finally {
// Only the last settling write reconciles, so an earlier resolving
// write cannot revert a still-pending sibling's optimistic value.
if (inFlight.current === 1) await load({ silent: true });
inFlight.current -= 1;
}
Was this helpful? React with 👍 / 👎
Code Review SummaryStatus: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit 7a87c2d)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 7a87c2d)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)
Reviewed by minimax-m3 · Input: 57.5K · Output: 7.1K · Cached: 325.5K |
|
Note Your trial team has used its Gitar budget, so automatic reviews are paused. Upgrade now to unlock full capacity. Comment "Gitar review" to trigger a review manually. Code Review 👍 Approved with suggestions 0 resolved / 1 findingsPrevents the 5s background poll from clobbering optimistic steer values by tracking in-flight writes. Consider extending the 💡 Edge Case: Reconcile load() can still revert a concurrent in-flight steer write📄 desktop/src/apps/ObservatoryApp.tsx:160-163 📄 desktop/src/apps/ObservatoryApp.tsx:140-145 The new This is a narrow case (requires rapid operation of two different controls), hence minor, but it undercuts the PR's "uniform across setScope/setGlobalCap/setLaneCap" claim. Consider only running the reconcile Skip the reconcile fetch while other steer writes are still in flight; the final write performs the authoritative reconcile.🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change:
Important Your trial ends in 3 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more. Was this helpful? React with 👍 / 👎 | Gitar |
![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 `@desktop/src/apps/ObservatoryApp.test.tsx`:
- Around line 359-365: The test validates that `postSteer` reconcile fetches the
fleet but does not verify that the 5-second polling interval actually resumes
after POST resolution. The current
`expect(fleetCalls()).toBeGreaterThan(before)` assertion passes because
`postSteer` itself calls `load()` in its finally block, not because polling
resumed. After the existing `Promise.resolve()` calls within the act block, add
another 5-second timer advancement (using jest fake timer methods), then add a
second assertion that fleetCalls() increases again to confirm the polling
interval has resumed independent of the reconcile operation.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e2f425a-6478-4e6d-b017-fc723ea4f544
📒 Files selected for processing (2)
desktop/src/apps/ObservatoryApp.test.tsxdesktop/src/apps/ObservatoryApp.tsx
| // Once the write resolves, postSteer's reconcile fetches the fleet. | ||
| await act(async () => { | ||
| resolvePost?.(); | ||
| await Promise.resolve(); | ||
| await Promise.resolve(); | ||
| }); | ||
| expect(fleetCalls()).toBeGreaterThan(before); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test currently validates reconcile fetch, not poll resumption.
expect(fleetCalls()).toBeGreaterThan(before) can pass even if the 5s interval never resumes, because postSteer itself calls load() in finally. Add one more 5s tick after resolving the POST and assert fleet calls increase again.
Suggested assertion tightening
await act(async () => {
resolvePost?.();
await Promise.resolve();
await Promise.resolve();
});
- expect(fleetCalls()).toBeGreaterThan(before);
+ const afterReconcile = fleetCalls();
+ expect(afterReconcile).toBeGreaterThan(before);
+
+ await act(async () => {
+ vi.advanceTimersByTime(5000);
+ await Promise.resolve();
+ });
+ expect(fleetCalls()).toBeGreaterThan(afterReconcile);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Once the write resolves, postSteer's reconcile fetches the fleet. | |
| await act(async () => { | |
| resolvePost?.(); | |
| await Promise.resolve(); | |
| await Promise.resolve(); | |
| }); | |
| expect(fleetCalls()).toBeGreaterThan(before); | |
| // Once the write resolves, postSteer's reconcile fetches the fleet. | |
| await act(async () => { | |
| resolvePost?.(); | |
| await Promise.resolve(); | |
| await Promise.resolve(); | |
| }); | |
| const afterReconcile = fleetCalls(); | |
| expect(afterReconcile).toBeGreaterThan(before); | |
| await act(async () => { | |
| vi.advanceTimersByTime(5000); | |
| await Promise.resolve(); | |
| }); | |
| expect(fleetCalls()).toBeGreaterThan(afterReconcile); |
🤖 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 `@desktop/src/apps/ObservatoryApp.test.tsx` around lines 359 - 365, The test
validates that `postSteer` reconcile fetches the fleet but does not verify that
the 5-second polling interval actually resumes after POST resolution. The
current `expect(fleetCalls()).toBeGreaterThan(before)` assertion passes because
`postSteer` itself calls `load()` in its finally block, not because polling
resumed. After the existing `Promise.resolve()` calls within the act block, add
another 5-second timer advancement (using jest fake timer methods), then add a
second assertion that fleetCalls() increases again to confirm the polling
interval has resumed independent of the reconcile operation.
…n-flight one Fold the gitar Edge-Case on the first version: the inFlight guard suppressed the background poll, but postSteer's reconcile load() still ran and, with two writes in flight, the first to finish read server state lacking the second and reverted its optimistic value. Move the steer-state adoption (pause/caps) inside load() behind inFlight === 0 so neither the poll nor a concurrent reconcile clobbers an optimistic value; decrement inFlight before the reconcile so only the last write's reconcile adopts. The fleet list still refreshes every time. New test drives a pause + cap concurrently and asserts the cap survives the pause's reconcile (the test also exercises both controls through the shared path, covering the kilo uniformity note). Deferred kilo nits: nil-risk increment ordering, and the dropped-tick which the reconcile already covers.
| const data = await throttleRes.json(); | ||
| setCap(coerceCap(data?.global)); | ||
| setLaneCaps(coerceLaneCaps(data?.lanes)); | ||
| if (inFlight.current === 0) { |
There was a problem hiding this comment.
SUGGESTION: The fix correctly prevents an earlier-resolving concurrent write from reverting a still-pending sibling's optimistic value, but the gating only fires while inFlight > 0. When the last write resolves, inFlight drops to 0 and its own reconcile runs adoption -- and if the server's fleet/throttle read has not yet observed that write (read-after-write lag, or the fleet endpoint aggregating from a different source than the POST target), the optimistic value is reverted by the very reconcile this PR was supposed to make safe. The new test only covers the cap-survives-while-pause-pending direction; the symmetric case (e.g., pause resolving last against a laggy fleet endpoint that still reports paused.global: false) is not asserted. Consider also gating adoption on the resolving write's seq (already tracked in steerSeq.current), or skipping adoption when the latest write was the one driving the reconcile.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| // The cap write's reconcile read says "no cap", but the pause write is still | ||
| // in flight, so the optimistic cap must be preserved (not reverted to 0). | ||
| expect(screen.getByLabelText(/concurrency cap value/i).textContent).toBe("1"); |
There was a problem hiding this comment.
SUGGESTION: The new test only asserts the cap survives while the pause POST is still pending (line 410). It does not assert the symmetric case -- pause surviving while the cap POST is still pending -- nor does it assert what happens after the pause POST resolves: the trailing await act(async () => { resolvePause?.(); await Promise.resolve(); }) at line 412 only awaits one microtask, so the pause's reconcile load() (which performs its own fleet + throttle fetches before adopting steer-state) is not flushed. The test therefore does not verify that the final reconcile correctly adopts the new pause state, and it leaves a real concern uncovered: if the server's fleet read has not yet propagated paused.global: true when the pause reconcile runs, the optimistic pause would be silently reverted back to false -- the exact class of flicker this PR is meant to eliminate.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
1 participant
What
Folds the deferred gitar Edge-Case from #1420: the 5s background poll (
load()over fleet + throttle) could land while a steer write was in flight and briefly revert the optimistic value (pause, global cap, or per-lane cap) before the POST completed.Change
inFlightref, incremented/decremented in the sharedpostSteer.inFlight > 0;postSteer's existing reconcileload()updates authoritatively once the write resolves.setScope/setGlobalCap/setLaneCap(all route throughpostSteer), so the three controls stay consistent.Tests
ObservatoryApp.test.tsxcase: with a pending POST, a 5s tick issues no background fleet fetch; once the POST resolves, the reconcile fetch runs. Plus the existing 13. All 14 green;tsc -bclean.Verification note
Behaviourally verified via vitest; visual check deferred to a live session, as with the other desktop slices.
Summary by CodeRabbit
Bug Fixes
Tests