Skip to content

fix(updater): ensure upgrade delivery service exits when p2p disabled - #473

Merged
qiuzhiqian merged 1 commit into
masterfrom
fix-delivery-start
Aug 17, 2026
Merged

fix(updater): ensure upgrade delivery service exits when p2p disabled#473
qiuzhiqian merged 1 commit into
masterfrom
fix-delivery-start

Conversation

@qiuzhiqian

@qiuzhiqian qiuzhiqian commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Calling the upgradedelivery ServiceStatus D-Bus method activates the service even when it is configured disabled, leaving it running after refresh. Add a helper that invokes DisableService and Clear on the disabled path in refreshUpgradeDeliveryService, and also trigger DisableService when SetP2PUpdateEnable(false) is called, so the delivery service exits properly.

Summary by Sourcery

Ensure the upgrade delivery (P2P) service is reliably shut down when it is disabled via configuration or D-Bus API.

Bug Fixes:

  • Stop leaving the upgrade delivery service running after refresh when P2P is configured as disabled.
  • Ensure disabling P2P updates via SetP2PUpdateEnable also triggers the upgrade delivery service to exit.

Enhancements:

  • Introduce a reusable helper to disable and clear the upgrade delivery service state and use it in both refresh and P2P toggle flows.
  • Add logging around upgrade delivery enablement decisions to aid in diagnosing service state issues.

@sourcery-ai sourcery-ai 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.

Sorry @qiuzhiqian, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@sourcery-ai

sourcery-ai Bot commented Aug 17, 2026

Copy link
Copy Markdown

Reviewer's Guide

Refactors upgrade delivery shutdown behavior so that the service is explicitly disabled and cleared whenever P2P delivery is configured off, ensuring the D-Bus-activated service exits even after status calls, and reuses a new helper from both the refresh logic and the P2P toggle path.

Sequence diagram for upgradedelivery shutdown when P2P is disabled

sequenceDiagram
    participant Updater
    participant UpdaterDBusAPI as Updater_dbus
    participant UpgradeDelivery as upgradedelivery_dbus

    UpdaterDBusAPI->>Updater: SetP2PUpdateEnable(sender, enable)
    Updater->>Updater: setPropP2PUpdateEnable(enable)
    alt enable is false and property changed
        Updater->>Updater: disableDeliveryService()
        Updater->>UpgradeDelivery: Call org.deepin.upgradedelivery.DisableService
        Updater->>UpgradeDelivery: Call org.deepin.upgradedelivery.Clear
    end

    Updater->>Updater: refreshUpgradeDeliveryService()
    Updater->>Updater: shouldEnableUpgradeDeliveryService(config, platformHasDelivery)
    alt shouldEnableService is false
        Updater->>Updater: disableDeliveryService()
        Updater->>UpgradeDelivery: Call org.deepin.upgradedelivery.DisableService
        Updater->>UpgradeDelivery: Call org.deepin.upgradedelivery.Clear
        Updater->>Updater: setPropP2PUpdateEnable(false)
    end
Loading

File-Level Changes

Change Details Files
Introduce a helper that encapsulates disabling and clearing the upgrade delivery D-Bus service and use it from the updater logic.
  • Add disableDeliveryService method that calls DisableService and Clear on org.deepin.upgradedelivery and wraps errors with context
  • Reuse the new helper in refreshUpgradeDeliveryService when delivery is configured off instead of inline DisableService/Clear calls
src/lastore-daemon/updater.go
Ensure the upgrade delivery service is shut down when P2P updates are turned off via the public D-Bus interface.
  • Change SetP2PUpdateEnable to only adjust the P2PUpdateEnable property and trigger delivery shutdown when disabling P2P
  • Call disableDeliveryService in SetP2PUpdateEnable when enable is false and the property actually changed, logging any failure
src/lastore-daemon/updater_ifc.go
Improve observability of delivery service enablement decisions.
  • Log shouldEnableService and serviceStatus inside refreshUpgradeDeliveryService to aid debugging of service state transitions
src/lastore-daemon/updater.go

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@github-actions

Copy link
Copy Markdown

CLA Assistant Lite bot:
提交邮箱中包含我们的合作伙伴,但您似乎并非合作伙伴的成员或对接人,请联系相关对接人将您添加至组织之中,或由其重新发起 Pull Request。
The commit email domain belongs to one of our partners, but it seems you are not yet a member of the current organization, please contact the contact person to add you to the organization or let them submit the Pull Request.

xml seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request

Calling the upgradedelivery ServiceStatus D-Bus method activates the
service even when it is configured disabled, leaving it running after
refresh. Add a helper that invokes DisableService and Clear on the
disabled path in refreshUpgradeDeliveryService, and also trigger
DisableService when SetP2PUpdateEnable(false) is called, so the
delivery service exits properly.

PMS: BUG-372269
@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

★ 总体评分:85分

■ 【总体评价】

代码成功修复了升级传递服务无法正常退出的逻辑缺陷,但残留了调试日志
逻辑正确且提取了公共方法,但因生产代码包含调试日志扣15分

■ 【详细分析】

  • 1.语法逻辑(完全正确)✓

新增的 disableDeliveryService 函数正确封装了 DBus 调用,使用 %w 包装错误保证了错误链完整。refreshUpgradeDeliveryService 函数去除了对 serviceStatus 的前置判断,直接调用关闭逻辑,精准修复了因 Status 接口拉起服务后无法退出的 BUG。SetP2PUpdateEnable 函数通过接收 changed 返回值,确保仅在状态实际发生变更时才触发 DBus 调用,逻辑严密。

  • 2.代码质量(一般)✕

代码通过提取 disableDeliveryService 方法消除了原有的重复代码,符合 DRY 原则,且添加了清晰的注释说明修改原因。但在 refreshUpgradeDeliveryService 函数中新增了一行 Info 级别的日志,直接打印内部变量 shouldEnableService 和 serviceStatus 的值,属于典型的调试信息未清理,影响了生产环境代码的整洁度。
潜在问题:生产环境日志中混入无意义的调试数据,增加日志存储负担并可能干扰问题排查
建议:移除该行调试日志,或将其降级为 Debug 级别

  • 3.代码性能(无性能问题)✓

修改后无论 serviceStatus 状态如何都会执行一次 DBus 调用,相比旧逻辑在某些分支下多了一次远程调用开销。但该函数仅在系统配置刷新时触发,调用频率极低,以极小的性能开销换取了服务状态一致性的关键 BUG 修复,属于合理的性能权衡。

  • 4.代码安全(存在0个安全漏洞)✓

漏洞对比统计:新增漏洞 0 个,减少漏洞 0 个,持平 0 个
代码仅涉及 DBus 接口的标准调用与状态判断,未引入命令注入、权限绕过或信息泄露等安全风险。新增的调试日志仅输出布尔值和枚举状态,不包含敏感系统数据,不构成安全漏洞。

■ 【改进建议代码示例】

diff --git a/src/lastore-daemon/updater.go b/src/lastore-daemon/updater.go
index d1c73c76c..e12345678 100644
--- a/src/lastore-daemon/updater.go
+++ b/src/lastore-daemon/updater.go
@@ -247,8 +247,6 @@ func (u *Updater) refreshUpgradeDeliveryService() {
 	}
 	shouldEnableService := shouldEnableUpgradeDeliveryService(u.config, platformHasDelivery)
-	logger.Infof("delivery shouldEnableService=%+v, serviceStatus=%+v", shouldEnableService, serviceStatus)
 	// 应用 UpgradeDeliveryEnabled 配置
 	if shouldEnableService {
 		// 配置启用:期望服务开启

@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: qiuzhiqian, zhaohuiw42

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@qiuzhiqian
qiuzhiqian merged commit 66f1ad9 into master Aug 17, 2026
23 of 28 checks passed
@qiuzhiqian
qiuzhiqian deleted the fix-delivery-start branch August 17, 2026 05:51
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.

3 participants