Skip to content

fix: spawn network tasks detached - #454

Merged
eudelins-zama merged 2 commits into
mainfrom
eudelins/fix/2929/detached-network-tasks
Mar 10, 2026
Merged

fix: spawn network tasks detached#454
eudelins-zama merged 2 commits into
mainfrom
eudelins/fix/2929/detached-network-tasks

Conversation

@eudelins-zama

@eudelins-zama eudelins-zama commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

Description of changes

  • Spawn network tasks detached, since the handles were only ever joined during shutdown (in Drop), which in practice means never.
  • Remove the ThreadHandleGroup entirely from the codebase.

Issue ticket number and link

Closes https://github.com/zama-ai/kms-internal/issues/2929

PR Checklist

I attest that all checked items are satisfied. Any deviation is clearly justified above.

  • Title follows conventional commits (e.g. chore: ...).
  • Tests added for every new pub item and test coverage has not decreased.
  • Public APIs and non-obvious logic documented; unfinished work marked as TODO(#issue).
  • unwrap/expect/panic only in tests or for invariant bugs (documented if present).
  • No dependency version changes OR (if changed) only minimal required fixes.
  • No architectural protocol changes OR linked spec PR/issue provided.
  • No breaking deployment config changes OR devops label + infra notified + infra-team reviewer assigned.
  • No breaking gRPC / serialized data changes OR commit marked with ! and affected teams notified.
  • No modifications to existing versionized structs OR backward compatibility tests updated.
  • No critical business logic / crypto changes OR ≥2 reviewers assigned.
  • No new sensitive data fields added OR Zeroize + ZeroizeOnDrop implemented.
  • No new public storage data OR data is verifiable (signature / digest).
  • No unsafe; if unavoidable: minimal, justified, documented, and test/fuzz covered.
  • Strongly typed boundaries: typed inputs validated at the edge; no untyped values or errors cross modules.
  • Self-review completed.

Dependency Update Questionnaire (only if deps changed or added)

Answer in the Cargo.toml next to the dependency (or here if updating):

  1. Ownership changes or suspicious concentration?
  2. Low popularity?
  3. Unusual version jump?
  4. Lacking documentation?
  5. Missing CI?
  6. No security / disclosure policy?
  7. Significant size increase?

More details and explanations for the checklist and dependency updates can be found in CONTRIBUTING.md

@cla-bot cla-bot Bot added the cla-signed The CLA has been signed. label Mar 9, 2026
@eudelins-zama
eudelins-zama marked this pull request as ready for review March 9, 2026 14:51
@eudelins-zama
eudelins-zama requested a review from a team as a code owner March 9, 2026 14:51
@eudelins-zama
eudelins-zama marked this pull request as draft March 9, 2026 14:51
@eudelins-zama eudelins-zama self-assigned this Mar 9, 2026
@eudelins-zama
eudelins-zama marked this pull request as ready for review March 9, 2026 14:52

@dvdplm dvdplm left a comment

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.

LGTM

@dvdplm

dvdplm commented Mar 9, 2026

Copy link
Copy Markdown
Contributor

For future self, here's the link to the internal discussion around this: https://zama-ai.slack.com/archives/C05UD8KC0UD/p1772530437428579

@maksymsur maksymsur left a comment

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.

LGTM

@github-actions

github-actions Bot commented Mar 9, 2026

Copy link
Copy Markdown

Consolidated Tests Results 2026-03-09 - 19:08:47

Test Results

passed 16 passed

Details

tests 16 tests
clock not captured
tool junit-to-ctrf
build build-and-test arrow-right test-reporter link #778
pull-request fix: spawn network tasks detached link #454

test-reporter: Run #778

Tests 📝 Passed ✅ Failed ❌ Skipped ⏭️ Pending ⏳ Other ❓ Flaky 🍂 Duration ⏱️
16 16 0 0 0 0 0 not captured

🎉 All tests passed!

Tests

View All Tests
Test Name Status Flaky Duration
nightly_full_gen_tests_k8s_default_threshld_sequential_crs 33.0s
test_k8s_threshld_insecure 3m 14s
k8s_test_crs_uniqueness 33.0s
k8s_test_insecure_keygen_encrypt_and_public_decrypt 3m 16s
k8s_test_insecure_keygen_encrypt_multiple_types 3m 36s
k8s_test_keygen_and_crs 3m 14s
k8s_test_keygen_uniqueness 8m 54s
full_gen_tests_k8s_default_threshld_sequential_crs 32.8s
test_k8s_threshld_insecure 3m 16s
k8s_test_crs_uniqueness 32.9s
k8s_test_keygen_and_crs 3m 15s
k8s_test_keygen_uniqueness 8m 52s
nightly_full_gen_tests_k8s_default_centralzd_sequential_crs 1.7s
test_k8s_centralzd_insecure 1m 4s
k8s_test_centralized_insecure 1m 1s
nightly_full_gen_tests_default_k8s_centralized_sequential_crs 1.7s

🍂 No flaky tests in this run.

Github Test Reporter by CTRF 💚

🔄 This comment has been updated

@titouantanguy titouantanguy left a comment

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.

LGTM ! Congrats finding this :)

Comment thread core/threshold/src/networking/sending_service.rs Outdated
Comment thread core/threshold/src/networking/sending_service.rs Outdated
Comment thread core/service/src/engine/centralized/central_kms.rs
@eudelins-zama
eudelins-zama merged commit cdcadd8 into main Mar 10, 2026
68 checks passed
@eudelins-zama
eudelins-zama deleted the eudelins/fix/2929/detached-network-tasks branch March 10, 2026 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed The CLA has been signed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants