Skip to content

TEST ONLY: concurrency cancel-in-progress (do not merge)#14

Closed
chethanuk wants to merge 4 commits into
mainfrom
ci/issue-422-concurrency
Closed

TEST ONLY: concurrency cancel-in-progress (do not merge)#14
chethanuk wants to merge 4 commits into
mainfrom
ci/issue-422-concurrency

Conversation

@chethanuk

Copy link
Copy Markdown
Owner

Fork-local test of the alibaba#422 concurrency block. Verifies that a superseded CI run is cancelled instead of sitting queued. Will be closed after verification.

chethanuk and others added 3 commits July 21, 2026 15:42
…rsation (alibaba#395)

* fix(llmloop): scope async memory compression to each RunPerFile conversation

The Runner is shared by all concurrent per-file review goroutines, but it
held a single compressionMu/pendingJob slot for async memory compression.
With concurrency > 1 that shared slot caused four defects (alibaba#384):

1. Cross-file apply: tryApplyPendingCompression had no owner check, so
   file B could splice file A's rebuilt history into its own messages.
2. Cross-file cancel/replace: the warning-threshold paths canceled
   whichever file's job happened to be pending, surfacing spurious
   "context canceled" errors at the gateway; triggerAsyncCompression
   overwrote the slot unconditionally, wasting the superseded request.
3. Same-call start-then-cancel: the soft-threshold trigger fired before
   the new messages were appended, so an append that crossed the warning
   threshold canceled the job started microseconds earlier.
4. The pendingJob == nil fast-path read in addNextMessage was unlocked.

Fix: move the bookkeeping into a compressionState owned by each
RunPerFile call and thread it through trigger/apply/cancel. The
nil-pending gate is now an atomic check-and-set inside
triggerAsyncCompression under st.mu, and the async trigger moved to the
end of addNextMessage, gated on the post-append count sitting strictly
between the soft and warning thresholds. RunPerFile defers a cancel so
no job outlives its conversation. Aggregate token counters and warnings
stay Runner-level.

Intentional behavior deltas, all strictly safer: the async snapshot now
includes the just-appended round; an in-flight job is canceled when
RunPerFile returns instead of running up to 5 minutes orphaned; no async
job starts when the call is about to return false.

Verified: new regression tests (cross-file isolation via a channel-gated
fake client, owner-only summary apply with post-snapshot suffix
preserved, no start-then-cancel in one update, 4 concurrent RunPerFile
calls under -race); all existing compression tests updated to the
per-conversation API; full suite green with -race; coverage 81.1%.
E2E: 2-file concurrent review against a local OpenAI-compatible stub
with compression exercised — zero "context canceled", both files done.

Fixes alibaba#384

* docs(llmloop): fix stale cancel-order comment, note sync-compression retry path

Review feedback on alibaba#395: cancelPendingCompression cancels and then
clears pendingJob, both under st.mu — the old comment described the
reverse order. Also note in addNextMessage that a pre-append
compression failure is retried by the post-append check.
…ors (alibaba#418)

* docs(telemetry): explain why checkMetricErr intentionally ignores errors

* style: remove trailing space and apply compact formatting
Every push to a PR started a new CI run while the previous one kept
running to completion on the self-hosted pool. Only the newest commit
matters, so the earlier runs were holding runners for results nobody
would read.

Adds a top-level concurrency group keyed on the PR number, falling back
to the ref for push-to-main. The block is byte-identical to the one
already in ocr-review.yml, which landed via alibaba#61 to fix the same problem
for the review workflow (alibaba#58); ci.yml is the workflow that never got it.

Left the other two workflows alone deliberately. deploy-pages.yml
already sets cancel-in-progress: false, and release.yml must never
cancel: aborting mid npm-publish would leave the platform packages
published while the meta package's optionalDependencies reference
versions that were never pushed, and npm publish is not reversible.

Verified with actionlint v1.7.12 (clean across all four workflows).

Closes alibaba#422
@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@chethanuk, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 58 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9243097a-4b4b-4dfb-8a44-1332ed762e02

📥 Commits

Reviewing files that changed from the base of the PR and between d0caa8a and 6c53195.

📒 Files selected for processing (5)
  • .github/workflows/ci.yml
  • internal/llmloop/compression.go
  • internal/llmloop/loop.go
  • internal/llmloop/runner_test.go
  • internal/telemetry/metrics.go
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ci/issue-422-concurrency

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chethanuk

Copy link
Copy Markdown
Owner Author

Test complete: superseded CI run cancelled in 38s (vs 24h queue baseline), and the OpenCodeReview run was correctly unaffected. Closing.

@chethanuk chethanuk closed this Jul 21, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the background memory compression mechanism in the LLM tool-use loop to scope it to individual conversations (per-file runs) rather than sharing a single global state on the Runner. This is achieved by introducing a compressionState struct that manages the mutex and pending job for each conversation, preventing concurrent file reviews from interfering with or cancelling each other's compression jobs. Extensive tests have been added to verify cross-file isolation, prevent race conditions, and ensure proper cancellation behavior. There are no review comments, so no feedback is provided.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants