fix(linear): back off on rate limits - #2
Open
shaunr0b wants to merge 6 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds tracker-required-labels and label-filtering in Linear client; classifies Linear GraphQL rate limits and computes orchestrator backoff; introduces slot-wait retry semantics with delay_type and snapshot visibility; updates dashboard/Presenter/LiveView and tests; small Codex handler for MCP elicitation requests. ChangesIssue Filtering and Linear Rate Limit Resilience
Orchestrator Retry Scheduling and Dashboard Rendering
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
elixir/lib/symphony_elixir/status_dashboard.ex (1)
315-321:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
tracker_backoffis dropped before render, so the new line never appears.The snapshot shaping logic omits
:tracker_backoffin bothsnapshot_with_samples/2andsnapshot_payload/0, soformat_tracker_backoff_line/1always seesnil.Suggested fix
{:ok, %{ running: running, retrying: retrying, codex_totals: codex_totals, rate_limits: Map.get(snapshot, :rate_limits), - polling: Map.get(snapshot, :polling) + polling: Map.get(snapshot, :polling), + tracker_backoff: Map.get(snapshot, :tracker_backoff) }}, @@ {:ok, %{ running: running, retrying: retrying, codex_totals: codex_totals, rate_limits: Map.get(snapshot, :rate_limits), - polling: Map.get(snapshot, :polling) + polling: Map.get(snapshot, :polling), + tracker_backoff: Map.get(snapshot, :tracker_backoff) }}Also applies to: 562-568
🤖 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 `@elixir/lib/symphony_elixir/status_dashboard.ex` around lines 315 - 321, The snapshot shaping code drops :tracker_backoff so format_tracker_backoff_line/1 always gets nil; update the snapshot builders (snapshot_with_samples/2 and snapshot_payload/0) to include the tracker_backoff value (e.g. add tracker_backoff: Map.get(snapshot, :tracker_backoff) or include the local tracker_backoff variable in the returned maps) so format_tracker_backoff_line/1 receives the real value; apply the same addition in the other snapshot construction site referenced around the second occurrence so both render paths include :tracker_backoff.
🤖 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 `@elixir/lib/symphony_elixir/linear/client.ex`:
- Line 143: fetch_issues_by_states/1 currently calls do_fetch_by_states(...,
nil, nil) which bypasses tracker.required_labels; change the call in
fetch_issues_by_states/1 so the label_filter argument is set to the tracker's
required_labels (or the result of a helper that builds the proper label filter)
instead of nil, i.e. pass tracker.required_labels (or
build_label_filter(tracker)) into do_fetch_by_states/4 so required labels are
enforced for this public path.
In `@elixir/lib/symphony_elixir/orchestrator.ex`:
- Around line 257-261: When Tracker.fetch_candidate_issues() succeeds we should
clear stale tracker_backoff immediately instead of only inside choose_issues/2;
update the with-chain so that after the {:ok, issues} <-
Tracker.fetch_candidate_issues() match you bind a new_state = %{state |
tracker_backoff: nil} (or otherwise set tracker_backoff: nil) and use new_state
for the subsequent available_slots(new_state) check and when calling
choose_issues(issues, new_state); apply the same change to the other occurrence
around lines 315-317 so successful fetches always reset tracker_backoff even if
available_slots/1 is false.
In `@elixir/test/symphony_elixir/core_test.exs`:
- Around line 705-710: The test spawns occupying_worker but may leak it if an
assertion fails; ensure the spawned process is always cleaned up by moving its
creation/cleanup into an on_exit callback (or wrap in try ... after) so you
always send :stop and/or monitor/kill the process; update the block that creates
occupying_worker (and the similar block around lines 751-753) to register
cleanup in on_exit/after referencing the occupying_worker variable so the
process is terminated regardless of test failures.
---
Outside diff comments:
In `@elixir/lib/symphony_elixir/status_dashboard.ex`:
- Around line 315-321: The snapshot shaping code drops :tracker_backoff so
format_tracker_backoff_line/1 always gets nil; update the snapshot builders
(snapshot_with_samples/2 and snapshot_payload/0) to include the tracker_backoff
value (e.g. add tracker_backoff: Map.get(snapshot, :tracker_backoff) or include
the local tracker_backoff variable in the returned maps) so
format_tracker_backoff_line/1 receives the real value; apply the same addition
in the other snapshot construction site referenced around the second occurrence
so both render paths include :tracker_backoff.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2ef0645-264c-47d3-80dc-0319bded63df
📒 Files selected for processing (12)
elixir/lib/symphony_elixir/codex/app_server.exelixir/lib/symphony_elixir/config/schema.exelixir/lib/symphony_elixir/linear/client.exelixir/lib/symphony_elixir/orchestrator.exelixir/lib/symphony_elixir/status_dashboard.exelixir/lib/symphony_elixir_web/live/dashboard_live.exelixir/lib/symphony_elixir_web/presenter.exelixir/test/support/test_support.exselixir/test/symphony_elixir/core_test.exselixir/test/symphony_elixir/extensions_test.exselixir/test/symphony_elixir/orchestrator_status_test.exselixir/test/symphony_elixir/workspace_and_config_test.exs
|
|
||
| true -> | ||
| do_fetch_by_states(project_slug, normalized_states, nil) | ||
| do_fetch_by_states(project_slug, normalized_states, nil, nil) |
There was a problem hiding this comment.
fetch_issues_by_states/1 is bypassing tracker.required_labels.
Line 143 passes nil for label_filter, so this public fetch path can return issues that other paths would correctly filter out.
Proposed fix
- do_fetch_by_states(project_slug, normalized_states, nil, nil)
+ do_fetch_by_states(project_slug, normalized_states, nil, required_label_filter())📝 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.
Suggested change
| do_fetch_by_states(project_slug, normalized_states, nil, nil) | |
| do_fetch_by_states(project_slug, normalized_states, nil, required_label_filter()) |
🤖 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 `@elixir/lib/symphony_elixir/linear/client.ex` at line 143,
fetch_issues_by_states/1 currently calls do_fetch_by_states(..., nil, nil) which
bypasses tracker.required_labels; change the call in fetch_issues_by_states/1 so
the label_filter argument is set to the tracker's required_labels (or the result
of a helper that builds the proper label filter) instead of nil, i.e. pass
tracker.required_labels (or build_label_filter(tracker)) into
do_fetch_by_states/4 so required labels are enforced for this public path.
Comment on lines
257
to
261
| with :ok <- Config.validate!(), | ||
| {:ok, issues} <- Tracker.fetch_candidate_issues(), | ||
| true <- available_slots(state) > 0 do | ||
| choose_issues(issues, state) | ||
| choose_issues(issues, %{state | tracker_backoff: nil}) | ||
| else |
There was a problem hiding this comment.
Clear stale tracker_backoff after successful tracker fetches.
tracker_backoff is only reset when choose_issues/2 runs. If fetch succeeds but available_slots(state) > 0 is false, stale backoff state persists.
Suggested fix
- with :ok <- Config.validate!(),
- {:ok, issues} <- Tracker.fetch_candidate_issues(),
- true <- available_slots(state) > 0 do
- choose_issues(issues, %{state | tracker_backoff: nil})
+ with :ok <- Config.validate!(),
+ {:ok, issues} <- Tracker.fetch_candidate_issues() do
+ state = %{state | tracker_backoff: nil}
+
+ if available_slots(state) > 0 do
+ choose_issues(issues, state)
+ else
+ state
+ end
else
@@
- false ->
- state
endAlso applies to: 315-317
🤖 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 `@elixir/lib/symphony_elixir/orchestrator.ex` around lines 257 - 261, When
Tracker.fetch_candidate_issues() succeeds we should clear stale tracker_backoff
immediately instead of only inside choose_issues/2; update the with-chain so
that after the {:ok, issues} <- Tracker.fetch_candidate_issues() match you bind
a new_state = %{state | tracker_backoff: nil} (or otherwise set tracker_backoff:
nil) and use new_state for the subsequent available_slots(new_state) check and
when calling choose_issues(issues, new_state); apply the same change to the
other occurrence around lines 315-317 so successful fetches always reset
tracker_backoff even if available_slots/1 is false.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tests
Summary by CodeRabbit
New Features
Tests