Skip to content

fix(cua-driver): drain daemon state on shutdown - #3348

Merged
r33drichards merged 6 commits into
mainfrom
fix/cua-driver-valgrind-errors
Sep 4, 2026
Merged

fix(cua-driver): drain daemon state on shutdown#3348
r33drichards merged 6 commits into
mainfrom
fix/cua-driver-valgrind-errors

Conversation

@r33drichards

@r33drichards r33drichards commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Stacking

Originally stacked on the Memcheck harness PR #3346, which merged to main as 692a63c. This branch is now rebased onto that merge and targets main directly; the six product commits plus the harness port are unchanged apart from the rebase.

Rebase onto current base (2026-09-04)

  • rebuilt on the current ci/cua-driver-valgrind tip, which pins the toolchain and runner, tracks stop_attempted, and validates through expect() instead of bare assert; the six product commits are cherry-picked unchanged and the Rust diff is byte-identical to the previous head apart from hunk offsets
  • the base branch replaced run-valgrind-e2e.sh with run-valgrind-e2e.py, so the harness changes from this stack (glibc stack-cache tunable, focused Memcheck flags, sessions list readiness, list_apps smoke call, NO_AT_BRIDGE=1) are ported to the Python runner in a dedicated commit with unit coverage
  • two unrelated Copilot-authored fleet commits that had been appended to this branch moved to fix(fleet): normalize proxied service path prefix #3543

Base findings

Base run https://github.com/trycua/cua/actions/runs/32673259741 exercised the real daemon and reported 29 possible-leak contexts / 16,184 bytes under strict Memcheck:

  • detached accepted-connection tasks retained SDK/session ownership after listener shutdown
  • standalone process session registries retained runtime-owned maps and callbacks
  • the daemon's Tokio runtime was not explicitly shut down
  • Linux AT-SPI and X11 clipboard process-lifetime threads started in a headless lane
  • the SDK lifecycle-maintenance and fallback recording cleanup threads were detached

Root-cause fixes

  • track accepted connection handlers in a JoinSet, abort and join them before SDK teardown
  • release standalone process session/proxy state after all connections stop
  • explicitly shut down the daemon Tokio runtime, with a Valgrind-tolerant 30-second join bound
  • make SDK lifecycle maintenance cancellable and joinable; avoid spawning fallback cleanup after explicit shutdown
  • stop creating the X11 clipboard background thread until a clipboard command actually needs it
  • honor NO_AT_BRIDGE=1 before constructing the persistent AT-SPI listener
  • disable glibc's joined-thread stack cache in the Memcheck harness; this changes allocator retention only and does not suppress errors or weaken leak flags
  • shut the internal Rust host's owned runtime down directly instead of initializing the process-global C ABI executor solely for shutdown
  • store the configured authorization registry behind releasable Arc state and clear it after daemon runtime shutdown
  • retain strict --leak-check=full --show-leak-kinds=definite,possible --errors-for-leak-kinds=definite,possible --error-exitcode=99 with no suppressions

Regression coverage

  • SDK test asserts explicit shutdown joins and clears lifecycle maintenance
  • Linux clipboard test asserts construction does not initialize the native clipboard thread
  • existing SDK coverage continues to assert idempotent shutdown and draining admitted calls
  • the same real-protocol daemon sequence remains in the stacked workflow

Validation and iterations

  • bash -n scripts/ci/linux/run-valgrind-e2e.sh
  • exact hosted command: dbus-run-session -- xvfb-run -a scripts/ci/linux/run-valgrind-e2e.sh libs/cua-driver/rust/target/debug/cua-driver
  • run 32675442579: 7 possible-leak contexts / 1,832 bytes
  • run 32677137469: 4 possible-leak contexts / 1,108 bytes
  • run 32677606750: 3 possible-leak contexts / 804 bytes
  • run 32678831547: 1 possible-leak context / 48 bytes
  • final strict run https://github.com/trycua/cua/actions/runs/32679202910: success with definitely lost: 0 bytes in 0 blocks, possibly lost: 0 bytes in 0 blocks, and ERROR SUMMARY: 0 errors from 0 contexts
  • all stacked PR checks are terminal: 24 passed, 1 intentionally skipped, 0 failed/cancelled/pending

Final three-context diagnosis

The 804-byte state came from three exact lifecycle owners rather than user-request allocations:

  • 48 bytes: configured SessionModeCeiling::process permission-mode set retained by the process-global authorization registry
  • 148 bytes: Tokio blocking-pool join-handle map retained because internal Rust shutdown initialized the static C ABI executor
  • 608 bytes: two Tokio blocking-worker TLS allocations owned by that same executor

Direct owned-runtime shutdown removed the 148/608-byte executor contexts. Releasing the configured authorization registry after daemon shutdown removed the final 48-byte context.

Current status

Strict end-to-end Memcheck and all relevant Linux, Windows, macOS, Nix, contract, formatting, docs, and metadata checks passed on the pre-rebase head c02ad4209a81578e1b1256e8f86409141f80e639. The rebased head re-runs the same checks; #3346 has landed and this PR is rebased onto it; the only other change on main since the tested head is an unrelated fleet billing commit.

@r33drichards

Copy link
Copy Markdown
Collaborator Author

Final CI verification: strict server E2E Memcheck is green with 0 definite bytes, 0 possible bytes, and 0 error contexts. All PR checks are terminal (24 passed, 1 intentional skip, 0 failed/cancelled/pending) on c02ad4209a81578e1b1256e8f86409141f80e639. The PR description now records the full finding progression and final lifecycle fixes.

injaneity
injaneity previously approved these changes Aug 24, 2026

@injaneity injaneity left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

shutdown ordering looks correct to me. connection tasks stop before sdk teardown, the tokio runtime is shut down, and process-global state is released afterward

@r33drichards
r33drichards force-pushed the ci/cua-driver-valgrind branch from 3b9d23f to ec3c4e9 Compare August 24, 2026 05:06
@r33drichards
r33drichards force-pushed the fix/cua-driver-valgrind-errors branch from c02ad42 to 0804c1d Compare August 24, 2026 05:06
f-trycua
f-trycua previously approved these changes Aug 24, 2026
@r33drichards
r33drichards force-pushed the ci/cua-driver-valgrind branch from ec3c4e9 to aae2fcd Compare August 24, 2026 19:17
@r33drichards
r33drichards force-pushed the fix/cua-driver-valgrind-errors branch from 0804c1d to 81706c9 Compare August 24, 2026 19:17
@r33drichards
r33drichards force-pushed the ci/cua-driver-valgrind branch from 3f93008 to 1203e26 Compare September 1, 2026 20:30
@r33drichards

Copy link
Copy Markdown
Collaborator Author

@copilot fix merge conflicts

Copilot AI commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

@copilot fix merge conflicts

Resolved by merging ci/cua-driver-valgrind into this branch and fixing the conflicts in commit b193165c.

@r33drichards

Copy link
Copy Markdown
Collaborator Author

@copilot fix merge conflicts

Copilot AI commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

@copilot fix merge conflicts

Fixed by merging ci/cua-driver-valgrind into this branch and resolving the conflicts in d16aa15c.

@r33drichards
r33drichards force-pushed the ci/cua-driver-valgrind branch 2 times, most recently from aa84401 to cdb6e44 Compare September 4, 2026 18:37
@r33drichards
r33drichards force-pushed the fix/cua-driver-valgrind-errors branch from 842ce8f to 1118cca Compare September 4, 2026 18:38
coding-sub-agent and others added 3 commits September 4, 2026 11:57
The base branch replaced run-valgrind-e2e.sh with run-valgrind-e2e.py,
so carry the harness changes from the shutdown-leak fixes over to the
Python runner:

- disable glibc's joined-thread stack cache via GLIBC_TUNABLES so
  retained stacks do not obscure ownership at exit
- drop --gen-suppressions=all and --num-callers=40 from the Memcheck
  command; the gate stays strict with no suppressions
- probe readiness through `sessions list --json` and require the
  empty-session contract from a fresh daemon
- exercise `list_apps` instead of `check_permissions` for the smoke call
- cover the command builders and environment setup in the unit tests

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CwPYqr5ukyx6JQGRLiATWq
@r33drichards
r33drichards changed the base branch from ci/cua-driver-valgrind to main September 4, 2026 18:57
@r33drichards
r33drichards dismissed stale reviews from f-trycua and injaneity September 4, 2026 18:57

The base branch was changed.

@r33drichards
r33drichards force-pushed the fix/cua-driver-valgrind-errors branch from 1118cca to a2a4a71 Compare September 4, 2026 18:58
@r33drichards
r33drichards merged commit aabb208 into main Sep 4, 2026
30 checks passed
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.

4 participants