Skip to content

fix(sd_notify): send Stopping on shutdown and use timeout/2 for watchdog interval - #433

Open
mohammadp1001 wants to merge 5 commits into
eclipse-opensovd:mainfrom
mohammadp1001:325-sd-notify-stopping-and-interval
Open

fix(sd_notify): send Stopping on shutdown and use timeout/2 for watchdog interval#433
mohammadp1001 wants to merge 5 commits into
eclipse-opensovd:mainfrom
mohammadp1001:325-sd-notify-stopping-and-interval

Conversation

@mohammadp1001

@mohammadp1001 mohammadp1001 commented Jul 11, 2026

Copy link
Copy Markdown

Summary

Follow-up to PR #322's post-merge review, addressing items 1, 2, and 3 from #325 in
cda-extra's systemd_notify implementation:

  • Send NotifyState::Stopping to systemd when the shutdown signal fires, so an
    intentional shutdown isn't mistaken for a watchdog failure.
  • Replace the timeout - 5s (floored to 1s) watchdog notify interval with
    timeout / 2, matching systemd's documented convention
    (sd_watchdog_enabled(3)).
    The previous formula collapsed to "always notify every 1s" for any realistic
    watchdog timeout, since .min(1s) was acting as a cap rather than a floor.
  • Renamed determine_interval to watchdog_notify_interval (item 2.1) to make
    its purpose clearer, and updated the doc comments accordingly.
  • Set default-features = false on the workspace sd-notify dependency in
    the root Cargo.toml. No functional change: sd-notify enables no
    features by default today.

Checklist

  • I have tested my changes locally
  • I have added or updated documentation
  • I have linked related issues or discussions
  • I have added or updated tests

Related

Follow-up to #322

@mohammadp1001
mohammadp1001 requested a review from a team as a code owner July 11, 2026 23:40
@github-actions github-actions Bot added the needs review Waiting for a review label Jul 11, 2026
…ndency

Follows the project's convention of minimizing pulled-in features. No
functional change: sd-notify does not enable any features by default today.

Item 3 of eclipse-opensovd#325.
@github-actions

github-actions Bot commented Jul 12, 2026

Copy link
Copy Markdown

📊 Coverage Report (unit + DoIP/CAN/mixed integration)

Total line coverage: 74.3%

Full build artifacts

@github-actions

github-actions Bot commented Jul 12, 2026

Copy link
Copy Markdown

⚠️ 2 CRAP regression(s) detected

↑ 2 regressed · ★ 1 new · ↔ 4 moved · ↓ 0 improved · 1707 unchanged · — 1 removed
✓ = clean, ▲ = moderate, ✗ = crappy; Δ = change since baseline; CC = cyclomatic complexity; Cov % = line coverage percentage

CRAP Δ CC Cov % Function Location
4.4 +0.4 4 70.8 UdsManager::start_variant_detection_for_ecus cda-comm-uds/src/variant.rs:43
6.4 +0.1 6 78.6 UdsManager::mark_group_unreachable cda-comm-uds/src/variant.rs:200
2.0 NEW 1 watchdog_notify_interval cda-extra/src/systemd_notify.rs:90
↔ 4 moved
CRAP Δ CC Cov % Function Location
42.0 6 trigger_watchdog cda-extra/src/systemd_notify.rs:101cda-extra/src/systemd_notify.rs
30.0 5 fold_status cda-extra/src/systemd_notify.rs:128cda-extra/src/systemd_notify.rs
12.0 3 create_sd_notify_task cda-extra/src/systemd_notify.rs:40cda-extra/src/systemd_notify.rs
2.0 1 fold_health_state cda-extra/src/systemd_notify.rs:143cda-extra/src/systemd_notify.rs
🔥 Top hot spots above threshold
CRAP CC Cov % Function Location
600.0 24 0.0 EcuManager::map_table_struct_from_uds cda-core/src/diag_kernel/payload_decode.rs:670
306.0 17 0.0 dop_variant_to_schema cda-core/src/diag_kernel/schema.rs:410
272.0 16 0.0 MbedtlsError::name comm-mbedtls/mbedtls-rs/src/error.rs:67
240.0 15 0.0 TlsStream::poll_read comm-mbedtls/mbedtls-rs/src/async_stream.rs:381
210.0 14 0.0 DoipDiagGateway::send_functional cda-comm-doip/src/lib.rs:648
210.0 14 0.0 UdsManager::send_functional_group cda-comm-uds/src/functional_group.rs:193
156.0 12 0.0 update_mdd_uncompressed cda-database/src/mdd_data.rs:364
156.0 12 0.0 has_new_artifacts cda-storage/src/recovery.rs:265
156.0 12 0.0 diag_service::functional_data_request cda-sovd/src/sovd/functions/functional_groups/data.rs:243
110.0 10 0.0 DiagServiceError::from cda-comm-doip/src/connections.rs:73
110.0 10 0.0 comparams::executions::id::get cda-sovd/src/sovd/components/ecu/operations.rs:273
90.0 9 0.0 params_to_schema cda-core/src/diag_kernel/schema.rs:188
90.0 9 0.0 add_phys_constr_range cda-core/src/diag_kernel/schema.rs:306
90.0 9 0.0 EcuManager::map_table_key_from_uds cda-core/src/diag_kernel/payload_decode.rs:622
90.0 9 0.0 establish_tls_ecu_connection cda-comm-doip/src/ecu_connection.rs:406
90.0 9 0.0 create_tls_stream cda-comm-doip/src/ecu_connection.rs:501
90.0 9 0.0 remove_descriptions_recursive cda-sovd/src/sovd.rs:923
85.5 16 35.2 service::executions::ecu_reset_handler cda-sovd/src/sovd/components/ecu/operations.rs:1346
72.0 8 0.0 TlsStream::async_handshake comm-mbedtls/mbedtls-rs/src/async_stream.rs:237
72.0 8 0.0 UdsManager::send_functional_to_gateway cda-comm-uds/src/functional_group.rs:39
72.0 8 0.0 UdsManager::set_functional_state cda-comm-uds/src/functional_group.rs:398
72.0 8 0.0 comparams::executions::id::put cda-sovd/src/sovd/components/ecu/operations.rs:401
72.0 8 0.0 generate_config_cmd cda-main/src/lib.rs:213
70.6 10 15.4 DiagDataValue::try_into cda-core/src/diag_kernel.rs:211
60.3 11 25.9 load_config_with_storage_override cda-main/src/config.rs:147

…and 38 more, see CI artifact for the full report.

— 1 removed
  • determine_interval (was 2.0) — cda-extra/src/systemd_notify.rs

Comment thread cda-extra/src/systemd_notify.rs Outdated
/// systemd-configured watchdog timeout, per the systemd-recommended convention
/// (see `sd_watchdog_enabled(3)`).
fn watchdog_notify_interval() -> Option<Duration> {
sd_notify::watchdog_enabled().map(|timeout| timeout / 2)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion: We should keep some safe guard, so Duration != Duration::Zero because tokio will panic when using Duration::Zero as tick interval

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mohammadp1001 Please do not resolve my threads yourself. I will resolve them if I agree with the solution. It helps tracking what I commented and what I reviewed, instead of going through the closed threads.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hi, Anything else missing?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

1ms interval will lead to considerable system load. It's legal from the systemd configuration as it supports intervals starting a 1 micro second. But imho it's not a sane configuration for the CDA, we should log a warning once.

@alexmohr alexmohr added changes requested Changes requested after a review and removed needs review Waiting for a review labels Jul 20, 2026
tokio::time::interval panics on a zero-length tick interval; clamp the
halved WatchdogSec to a minimum of 1ms.
@github-actions github-actions Bot added needs review Waiting for a review and removed changes requested Changes requested after a review labels Aug 4, 2026
@theswiftfox

Copy link
Copy Markdown
Contributor

@mohammadp1001 can you check the pipeline failure and fix the reported clippy lint?

@mohammadp1001

Copy link
Copy Markdown
Author

Sure, I will check.

Plain division tripped clippy::arithmetic_side_effects, which is
deny-level workspace-wide. The divisor is a non-zero literal, so the
fallback is unreachable and behaviour is unchanged.
@alexmohr alexmohr added changes requested Changes requested after a review and removed needs review Waiting for a review changes requested Changes requested after a review labels Aug 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changes requested Changes requested after a review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants