fix(process): serialize non-leader exec handoff#2156
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Root-cause analysis and fix are complete for the deterministic Integration Test regression. The failure in Commit 675667c fixes this at the accounting boundary:
Validation completed:
Three independent adversarial reviews checked Linux 6.6 semantics, lifecycle ordering, lock ordering, double/lost accounting, performance, overflow, security, and responsibility boundaries. No remaining actionable issue was found. A temporary suggestion to pre-account the old leader was rejected after tracing the complete release sequence because it would double-count that task. The PTY SIGHUP result was also investigated separately. No causal path from this PR to the TTY/signal implementation was found, and focused |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 675667c41a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Root cause fixed in 6cb2bc8. DragonOS stores every thread in the PIDTYPE_PGID list, while Linux stores only thread-group leaders. send_signal_to_pgid() therefore delivered one process-directed signal per thread. In the failing pty sequence, a blocked sibling caused an extra shared-pending SIGHUP, which later terminated ReleaseTTYSignals. The fix selects physical leader entries and deduplicates transient leaders by stable TGID identity; it does not alter Linux-compatible TIOCNOTTY behavior. Added a realtime-signal regression that verifies one process-group send creates exactly one pending signal per thread group. Verified with make kernel, the targeted dunitest in DragonOS, and the full gVisor pty_test (86 passed, 1 skipped). |
|
@codex review |
|
The follow-up format-only CI failure was caused by one rustfmt line-wrap mismatch in kernel/src/ipc/kill.rs. Commit bc337b1 applies the exact canonical formatting with no logic change. The repository-equivalent |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc337b1c97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
a02db36 to
bc337b1
Compare
|
Root cause and fix update: The pty failure was caused by modeling PGID/SID membership per thread while process-group signals are process-directed. A multithreaded process therefore appeared multiple times in the physical PGID index, and SIGHUP was queued repeatedly in the shared pending set. This update makes PGID/SID membership leader-representative, preserves TGID existing per-thread consumers, and transfers the representative atomically when a non-leader execs. Fork, setpgid/setsid, de_thread, exit, and group traversal now share the same membership serialization. The TTY/job-control paths were also tightened so controlling-session ownership, foreground PGID changes, vhangup, and final thread-group exit are serialized and use expected-owner removal. This avoids stale teardown, rebind races, and clearing a foreground process group that still has live members. The two review findings were checked against the complete lifetime paths:
Validation completed:
A full local guest dunitest retry was blocked before QEMU startup by the external DragonOS source mirror while downloading coreutils-9.4.tar.xz; this is unrelated to the code under test. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a02db360e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
707c1e0 to
2ef2c6d
Compare
|
Follow-up CI root cause fixed in 2ef2c6d: the failing FatalSignalDuringSiblingExecKeepsWaitOwnership case exposed that thread-directed fatal signals skipped the fatal group-exit transition, while the default SIGKILL handler treated every nonleader target as a thread-only exit. If tgkill reached the execing pthread before de_thread promoted it, only that pthread died and the original leader remained paused, so the parent could never reap the process. The fix now starts group exit for external fatal PID-targeted signals, matching Linux complete_signal semantics, and routes only the kernel-private sibling teardown used by de_thread and an already committed group exit through a dedicated private SIGKILL primitive. Two adversarial reviewers approved the GROUP_EXEC/GROUP_EXIT interleavings, pending publication, wait ownership, and lock ordering. Format/Clippy and make kernel pass locally; the targeted guest retry was blocked before QEMU by the external DragonOS musl mirror, so the newly triggered CI run is the authoritative guest verification. |
|
@codex review |
d8b4ff7 to
a02db36
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ef2c6dca3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Final-head CI update for
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31d2262e5a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
Model de_thread as an owned group-exec transaction so the old leader exit, fatal cancellation, PID identity migration, and natural-parent notification have a single serialized handoff. Exchange task, struct pid, global raw-PID, cgroup, and ptrace indices under a fixed lock order. Delay wait and pidfd visibility until identity teardown completes, preserve ptrace exit-signal semantics, and keep natural-parent notification ownership per thread-group leader. Add deterministic PID/pidfd coverage, fatal-signal sibling-exec stress, ptrace clone exit-signal coverage, and retain the existing 512-iteration spawn/exec pipe race. Tests: cargo clippy (kernel, all features) Tests: make kernel Tests: spawn_exec_pipe_race_test (3/3) Tests: wait_rusage_test (34/34) Tests: process_signal_fork_test (5/5) Tests: proc_self_exec_cmdline_test (2/2) Signed-off-by: longjin <longjin@dragonos.org>
Keep raw nanosecond CPU accounting for threads after they leave a thread group so CLOCK_PROCESS_CPUTIME_ID continues to include joined workers without timeval precision loss. Serialize the exited accumulator with thread-group membership changes, and carry exited-thread plus child resource history across the non-leader exec leader handoff. The old leader's own usage remains accounted exactly once by the post-handoff release path. Signed-off-by: longjin <longjin@dragonos.org>
DragonOS links every thread into PIDTYPE_PGID, unlike Linux which links only thread-group leaders. Process-group delivery therefore sent one process-directed signal per thread, allowing a blocked sibling to leave a duplicate shared pending SIGHUP that later terminated pty_test. Select physical leader entries, deduplicate transient leaders by stable TGID identity, and prefer the live leader during de_thread handoff. Add a realtime-signal regression test that proves one killpg call creates exactly one pending instance per thread group. Signed-off-by: longjin <longjin@dragonos.org>
Signed-off-by: longjin <longjin@dragonos.org>
Represent PGID and SID membership once per thread group while retaining per-thread TGID links required by existing consumers. Transfer leader-owned links and session state atomically during de_thread, and serialize fork, setpgid, setsid, exit, and process-group lookup against membership changes. Make process-group signal delivery target one representative per thread group. Keep controlling-TTY publication, removal, hangup, and foreground-group changes in ordered transactions so stale owners cannot clear or signal a replacement session. Perform session teardown only for the unique last live thread. Add dunitest coverage for inherited PGID/SID across pthread creation, single process-group realtime-signal delivery, and session leadership after a nonleader exec. Signed-off-by: longjin <longjin@dragonos.org>
Start fatal group exit for thread-directed signals as Linux does, so tgkill(SIGKILL) cannot terminate only the selected pthread and leave its process leader alive. Separate the private SIGKILL used by de_thread and an already-committed group exit from ordinary signal delivery. This keeps exec sibling teardown local without confusing external fatal signals or re-entering group-exit selection. Signed-off-by: longjin <longjin@dragonos.org>
Keep the old leader's children index and every child parent link in one tasklist-like relationship transaction during non-leader exec. Make release select the current real parent and remove the matching children entry under the same lock, preventing stale virtual PIDs when child reap races the handoff. Reuse the locked reparent helper and remove the now-redundant wrapper. Signed-off-by: longjin <longjin@dragonos.org>
Guard the process-group helper and child immediately after fork so fatal test assertions cannot leave a paused helper or a gate-blocked child behind. Disarm each guard only after its normal waitpid succeeds, preserving the existing success path while keeping later dunitest cases isolated from failure-path processes. Signed-off-by: longjin <longjin@dragonos.org>
Build child-exit siginfo from the explicitly notified group leader instead of the thread that happens to complete the notification transaction. Map the leader PID and UID through the parent's namespaces and report Linux-compatible CLD status and CPU times. Add a deterministic multithreaded regression test that keeps the leader and final worker on one CPU, waits for the leader to become a zombie, differentiates the worker credentials, and verifies the parent's SIGCHLD metadata. Signed-off-by: longjin <longjin@dragonos.org>
Publish the natural-parent notification transaction before delivering the child exit signal so a concurrently scheduled SIGCHLD handler cannot observe the internal Pending phase and miss a nonblocking wait. Snapshot siginfo before autoreap can unhash the child identity, complete autoreap before publishing Done, and retain the unconditional parent wait-queue wake after signal delivery. Assert the notification owner's Zombie-to-Dead transition instead of silently leaving an inconsistent autoreap state. Add a dunitest regression whose SIGCHLD handler immediately reaps the expected child with waitpid(WNOHANG), including bounded observation and failure-safe child and signal-action cleanup. Signed-off-by: longjin <longjin@dragonos.org>
Keep the post-exec image blocked so a successful exec cannot turn a lost thread-targeted SIGKILL into a normal exit. Track retirement of the exchanged TID until wait completion and only release the image after ESRCH proves that the old identity disappeared. Require every observable fatal delivery to terminate the process with SIGKILL. Signed-off-by: longjin <longjin@dragonos.org>
Publish the shared fatal group-exit code under the sighand lock before exposing private SIGKILL pending state. This prevents de_thread cancellation from racing ahead and replacing SIGKILL with EAGAIN. Report the shared group-exit status consistently through zombie waits and natural-parent SIGCHLD notification, matching Linux observable semantics. Signed-off-by: longjin <longjin@dragonos.org>
Publish the completed natural-parent notification before taking a fresh parent snapshot under the ptrace relation lock. Wake both the original notification owner and a different completion-time parent so a concurrent zombie reparent cannot strand wait4 after observing the transient Pending state. Signed-off-by: longjin <longjin@dragonos.org>
Keep the Linux pty_close driver callback for the logical last user descriptor, but clear the controlling-session association only on a true final tty release. Add a dunitest that closes the last slave descriptor while the master remains alive and verifies that the session can still reopen /dev/tty with the original SID and foreground process group. Signed-off-by: longjin <longjin@dragonos.org>
Replace the exit-time scan of every thread-group member with a shared live-task counter, matching Linux signal_struct::live semantics. Increment the counter only when a CLONE_THREAD task is published and consume exactly one token when each task enters exit, so the unique 1-to-0 transition owns group-finalization work. Keep the counter in ThreadInfo rather than SigHand because CLONE_SIGHAND can be shared by distinct thread groups. This removes quadratic exit behavior and shortens the global PID membership lock's irq-disabled critical section without changing group-exec or finalization ordering. Refs: DragonOS-Community#2153 Signed-off-by: longjin <longjin@dragonos.org>
a0e7606 to
9af39f0
Compare
|
CI root-cause update after rebasing onto origin/master@e374b4513: The Integration Test failure in BasicPtyTest.OpenDevTTY is not an open(/dev/tty) failure. InForkedProcess returns the raw wait status, and the observed value 1 means the child was terminated by SIGHUP. The descriptor cleanup order closes the PTY master last; Linux 6.6 pty_close() calls tty_vhangup() on the slave and sends SIGHUP to its foreground process group. DragonOS now does the same. Reverting the ctty lifetime fix, clearing the controlling tty at the slave logical-close count, or suppressing the hangup would be a kernel workaround that violates Linux semantics. Upstream gVisor corrected this exact stale-test assumption in google/gvisor@1686126 by installing SIG_IGN for SIGHUP in BasicPtyTest.OpenDevTTY and adding a positive SIGHUPOnMasterClose test: google/gvisor@1686126 DragonOS currently consumes the release_20251218 prebuilt test archive. I also downloaded and ran the newer available release_20260525 archive; its pty_test still contains the old test and fails with the same raw status 1 on host Linux. Therefore there is no correct DragonOS kernel patch for this Integration failure: the DragonOS test-suites artifact must be rebuilt from a gVisor revision containing 16861266b943. Skipping the test locally was intentionally avoided. The valid P2 review finding was fixed in 9af39f0 with a Linux-style live thread-group counter. Post-rebase validation passed FMT_CHECK=1 make fmt, make kernel, ExitTest.Success, and ExitTest.ExitAllThreads. BasicPtyTest.OpenDevTTY was rerun only to confirm the expected stale-test SIGHUP diagnosis. |
|
@codex review |
Hold a strong reference to the PTY master before publishing an in-flight slave open. Without this pin, the last master close can drop the weak peer between begin_slave_open() and pty_common_open(), exposing an internal ENODEV race to userspace. Return EIO when the peer is already gone, matching Linux pts open semantics, while preserving the existing opening/active accounting that prevents premature devpts index reuse. Signed-off-by: longjin <longjin@dragonos.org>
|
Additional post-rebase TTY finding fixed in ebd7ccf: The latest master adds ConcurrentSlaveOpenAndMasterCloseNeverReusesLiveIndex. A focused local dunitest run exposed an intermittent ENODEV that was not caused by the controlling-tty change: the slave TtyCore stores its master link as Weak, and the old open path called begin_slave_open() before pty_common_open() upgraded that link. The last master close could drop the final master Arc in that window, so pty_common_open() observed no peer and leaked internal ENODEV. The fix pins the master with a strong Arc before publishing the in-flight slave open. If the peer is already gone, the open returns EIO, which matches Linux pts semantics. If the open wins the race, the pin keeps the pair alive while the existing begin/finish/abort accounting prevents premature index reuse. This does not delay master-close state publication, hangup, or unlink. Validation after the fix:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebd7ccf972
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Deliver ptrace exit notifications with a child-layout siginfo instead of a generic SI_USER payload. Snapshot status, namespace-visible identity, and exit accounting before publishing Zombie so a polling tracer cannot reap and unhash the task first. Match Linux exit accounting by combining the exiting task with already released siblings while excluding live threads, and add a dunitest regression for the observable SIGCHLD fields. Signed-off-by: longjin <longjin@dragonos.org>
d231734 to
5006c46
Compare
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
exec/de_threadas an owned, generation-based group-exec transaction that serializes sibling teardown, old-leader exit, fatal cancellation, identity exchange, and observer visibilityFixes #2153
Design and implementation
Non-leader exec handoff
Pending/Exiting/Readyold-leader phases and per-task generation tokens to the shared group-exec transactionchildrenindex in the same tasklist-like relationship transactionExit, wait, signal, and accounting semantics
1 -> 0transition owns finalization work without an exit-time group scanSIGKILL, and preserve Linux process-fatal semantics for externally delivered thread-directed fatal signalsSIGCHLDZombiepublication, avoiding reap/unhash racesProcess groups, sessions, and TTY/PTTY lifetime
de_thread, and serialize fork/setpgid/setsid/exit readers against membership updatesEIOif the peer is already goneRegression coverage
The dunitest changes cover:
SIGCHLDhandler reap, and reparent wake orderingSIGCHLDmetadata and ptrace exit signal/siginfo behavior/proc/selfexec-visible identityValidation
cargo clippyfor the kernel with all featuresmake kernel/proc/selfexec identity, and TTY/PTTY hangupKnown CI limitations
BasicPtyTest.OpenDevTTYcleanup expectation: closing the PTY master correctly produces Linux-compatibleSIGHUP; the gVisor test artifact needs the upstream test fix rather than a DragonOS kernel workaroundpipe_testandsocket_ip_tcp_generic_loopback_test; the immediately preceding run used the identical source tree and did not reproduce those timeoutsclaude-reviewis rejected before checkout because its trustedpull_request_targetworkflow attempts to check out fork code; this is a workflow security-policy failure, not a source failure