Skip to content

Conversation

@jbedard
Copy link
Member

@jbedard jbedard commented Oct 30, 2025

This way binaries such as aspect_gazelle_runner or formatters can specify they want to receive CYCLE events for any source changes, not only runfiles changes like today.

Changes are visible to end-users: no

Test plan

  • Covered by existing test cases
  • Manual testing

Copilot AI review requested due to automatic review settings October 30, 2025 18:01
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces protocol versioning and an explicit subscription model for the incremental build protocol. The changes enable clients to negotiate protocol versions and explicitly subscribe to specific watch types (sources or runfiles).

Key changes:

  • Adds protocol version negotiation with support for version 0 (legacy) and version 1 (with explicit subscribe)
  • Refactors AwaitCycle() to Subscribe(options) returning iter.Seq2 for proper error handling
  • Introduces WatchType enum to allow subscribing to specific change types (sources/runfiles)

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
runner/pkg/ibp/protocol.go Introduces ProtocolVersion type, version constants, WatchType enum, and SubscribeMessage structure
runner/pkg/ibp/client.go Refactors client to negotiate protocol versions and implement explicit subscription with error handling via iter.Seq2
runner/runner.go Updates Watch method to use new Subscribe API with explicit watch type
runner/pkg/ibp/BUILD.bazel Adds dependency on common/logger package

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@jbedard jbedard force-pushed the ibp-subscribe-options branch from c60fbce to 10cc314 Compare October 30, 2025 19:26
@aspect-workflows
Copy link

aspect-workflows bot commented Oct 30, 2025

Test

All tests were cache hits

3 tests (100.0%) were fully cached saving 233ms.


Test

language/js

All tests were cache hits

117 tests (100.0%) were fully cached saving 20s.


Test

language/kotlin

All tests were cache hits

19 tests (100.0%) were fully cached saving 3s.


Test

language/orion

All tests were cache hits

49 tests (100.0%) were fully cached saving 10s.


Test

runner

9 test targets passed

Targets
//tests:bad_build_test [k8-fastbuild]229ms
//tests:branded-directives_test [k8-fastbuild]324ms
//tests:crossresolve-js_test [k8-fastbuild]364ms
//tests:crossresolve-pnpm-names-lazy_test [k8-fastbuild]413ms
//tests:crossresolve-pnpm-names_test [k8-fastbuild]398ms
//tests:crossresolve-pnpm_test [k8-fastbuild]255ms
//tests:imports-missing_test [k8-fastbuild]362ms
//tests:js_binary-main_test [k8-fastbuild]364ms
//tests:npm_bin_wrapper_test [k8-fastbuild]342ms

Total test execution time was 3s. 15 tests (62.5%) were fully cached saving 1s.


Buildifier      Gazelle      Gazelle [language/js]      Gazelle [language/kotlin]      Gazelle [language/orion]      Gazelle [runner]

Copilot AI review requested due to automatic review settings October 30, 2025 19:43
@jbedard jbedard force-pushed the ibp-subscribe-options branch 2 times, most recently from 66e6ef2 to a07da35 Compare October 30, 2025 19:44
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@aspect-build aspect-build deleted a comment from Copilot AI Oct 30, 2025
@aspect-build aspect-build deleted a comment from Copilot AI Oct 30, 2025
@jbedard jbedard force-pushed the ibp-subscribe-options branch from a07da35 to 294db47 Compare October 30, 2025 23:01
Copilot AI review requested due to automatic review settings November 1, 2025 21:23
@jbedard jbedard force-pushed the ibp-subscribe-options branch from 294db47 to 43098e9 Compare November 1, 2025 21:23
@jbedard jbedard requested a review from thesayyn November 1, 2025 21:23
@jbedard jbedard marked this pull request as ready for review November 1, 2025 21:23
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


type SubscribeMessage struct {
Message
WatchType WatchType `json:"watch_type"`
Copy link
Member Author

Choose a reason for hiding this comment

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

Options:

  • single type
  • multiple bools (source + runfiles)
  • array of types

If a binary says it only wants to subscribe to sources should it be restarted if it's runfiles change? Or we should ignore runfiles? If watching both should should they be distinguished in the CYCLE message somewhere such as a sources[path]: {type: 'runfiles' | 'sources'} flag? :/

@jbedard
Copy link
Member Author

jbedard commented Nov 1, 2025

rules_js support: aspect-build/rules_js#2411

@jbedard
Copy link
Member Author

jbedard commented Nov 20, 2025

This will instead be done with a CAP(abilities) request+response instead and sticking with a single subscription per socket connection.

@jbedard jbedard closed this Nov 20, 2025
@jbedard
Copy link
Member Author

jbedard commented Nov 24, 2025

Will be doing the original protocol CAP message instead

@jbedard jbedard deleted the ibp-subscribe-options branch November 24, 2025 09:22
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