Skip to content

fix(BA-3162): Remove agent heartbeat handling observation from audit log#6987

Open
seedspirit wants to merge 1 commit intomainfrom
fix/BA-3162
Open

fix(BA-3162): Remove agent heartbeat handling observation from audit log#6987
seedspirit wants to merge 1 commit intomainfrom
fix/BA-3162

Conversation

@seedspirit
Copy link
Contributor

resolves #6966 (BA-3162)

Checklist: (if applicable)

  • Milestone metadata specifying the target backport version
  • Mention to the original issue
  • Installer updates including:
    • Fixtures for db schema changes
    • New mandatory config options
  • Update of end-to-end CLI integration tests in ai.backend.test
  • API server-client counterparts (e.g., manager API -> client SDK)
  • Test case(s) to:
    • Demonstrate the difference of before/after
    • Demonstrate the flow of abstract/conceptual models with a concrete implementation
  • Documentation
    • Contents in the docs directory
    • docstrings in public interfaces and type annotations

Copilot AI review requested due to automatic review settings November 27, 2025 09:24
@github-actions github-actions bot added size:XS ~10 LoC comp:manager Related to Manager component labels Nov 27, 2025
@seedspirit seedspirit added this to the 25.15 milestone Nov 27, 2025
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 removes HandleHeartbeatAction.spec() from the list of supported actions in the agent processors, effectively excluding agent heartbeat handling from being listed as a supported action. This aligns with the treatment of other internal system actions.

  • Removes HandleHeartbeatAction from the supported_actions() method return list
  • Maintains the actual heartbeat processing functionality (processor still initialized and functional)
  • Follows the existing pattern where internal actions like mark_agent_exit and mark_agent_running are also excluded from supported_actions()

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

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

Copy link
Collaborator

@HyeockJinKim HyeockJinKim left a comment

Choose a reason for hiding this comment

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

Shouldn't we be looking at the part where we're applying action monitors, rather than removing the spec?

Copy link
Collaborator

@HyeockJinKim HyeockJinKim left a comment

Choose a reason for hiding this comment

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

The implementation is wrong.

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 6 out of 6 changed files in this pull request and generated 3 comments.


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

Copy link
Collaborator

@HyeockJinKim HyeockJinKim left a comment

Choose a reason for hiding this comment

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

This seems to be the wrong approach.

Copy link
Member

@fregataa fregataa left a comment

Choose a reason for hiding this comment

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

  1. Using negative phrasing like "SKIP_AUDIT_LOG" in enum names can lead to readability issues, especially when used in conditional statements. But in this case it does not harm readability that much
  2. All monitors will be skipped during Heartbeat action handling. Is there any monitors that Heartbeat should be done?

@HyeockJinKim
Copy link
Collaborator

I believe that the method is wrong, and it can be applied by excluding the monitor at the time of registration, but adding an additional flag field rather increases complexity. This is a problem that needs to be handled differently during registration, not by adding tags or the like and applying them differently after registration.

Copy link
Member

@fregataa fregataa left a comment

Choose a reason for hiding this comment

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

In my opinion, the action should determine whether it should be excluded by a certain rather than the processor. How do you think? @HyeockJinKim

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

Labels

comp:manager Related to Manager component size:L 100~500 LoC

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove agent heartbeat handling observation from audit log

4 participants