archive - zygaena/zyginit

root / bugs / archive
archive Plain Text 465 lines 21.6 KB
> Note (2026-05-21): The `console-login.toml` referenced below is
> now in Hammerhead's overlay tree at
> `base/usr/src/zyginit/overrides/console-login/service.toml`.

# Bug 001: supervisor leaves /dev/console as inherited fd 0 in service children

## Summary

`src/supervisor.reef`'s child-setup path redirects fd 1 and fd 2 to the
per-service log file but leaves fd 0 inherited from PID 1. PID 1 (zyginit)
runs `setup_pid1_console()` which opens `/dev/console` and dup2's the fd
onto 0/1/2 (with `O_NOCTTY`, correctly), so any service child inherits
fd 0 = `/dev/console`. The child then calls `setsid()`, becoming a session
leader with a `/dev/console` fd open.

That state — session leader with a tty fd inherited from before the
session was created — appears to be enough on Hammerhead for the kernel
to associate `/dev/console` as the new session's controlling tty even
though no explicit `open()` (with or without `O_NOCTTY`) was made by
the child for that fd. Symptom: `ps -o tty` shows `TT=console` for the
service, and any later daemon trying to claim ctty on `/dev/console`
(typically `ttymon` started by the `console-login` service in a higher
tier) fails with **"cannot allocate controlling tty on /dev/console"**
and login(1) cannot proceed → `console-login` enters a restart loop and
the system is unreachable from the physical console.

This was hit on hh-alpha1 immediately after first boot. SSH worked,
console login did not.

## Severity

**Medium.** Affects any deployment where the kernel auto-claims ctty
under the inherited-fd-0-then-setsid pattern. The triggering daemons
are kernel-version- and toolchain-build-dependent (we observed it with
a newly-built `syslogd` on hh-alpha1; an older syslogd on hh-prototest
does not trip the same kernel path). Recoverable via SSH but blocks
the physical console use case.

## Affected Version

zyginit 0.1.1 (as shipped in `zyginit-0.1.1-source.tar.xz`).

The defect is in `src/supervisor.reef`, the post-fork child setup near
line 333 in the version we vendored. We did not check 0.1.0.

## Reproducer

1. Boot Hammerhead with zyginit as PID 1.
2. Have `setup_pid1_console()` open `/dev/console` and dup it onto 0/1/2.
3. Start any daemon that calls `setsid()` and continues running with
   the inherited fd 0 in scope (e.g. `/usr/sbin/syslogd`).
4. Inspect: `ps -ef -o pid,sid,tty,comm | grep <daemon>`.
   The TT column shows `console` even though the daemon never explicitly
   opened `/dev/console` without `O_NOCTTY`.
5. Start `console-login` (or any service running `ttymon -d /dev/console`)
   in a higher tier. ttymon prints the login prompt but `login(1)` then
   fails to claim ctty: "cannot allocate controlling tty on /dev/console
   — There may be another session active on this port."
6. zyginit restarts `console-login`; same failure repeats; restart loop.

## Root cause (from the supervisor side)

`src/supervisor.reef` post-fork child setup (snippet from the version
we vendored):

```reef
if pid == 0
    // CHILD PROCESS
    process.process_setsid()

    // Redirect stdout/stderr to per-service log file
    let log_path = ...
    let log_fd = fd.fd_open(log_path, log_flags, 420)
    if log_fd >= 0
        fd.fd_dup2(log_fd, 1)
        fd.fd_dup2(log_fd, 2)
        fd.fd_close(log_fd)
    end if
    ...
```

Only fd 1 and fd 2 get redirected. fd 0 keeps PID 1's `/dev/console`
across the `setsid()`. From the kernel's perspective the new session is
born with an open fd referencing a tty that has no controlling session,
which under at least some illumos variants is enough to associate it
as the new session's ctty.

## Suggested fix

In the child-setup path, before `setsid()`, redirect fd 0 to `/dev/null`
(or to the per-service log file, or close it entirely). For example:

```reef
if pid == 0
    // CHILD PROCESS

    // Redirect fd 0 to /dev/null BEFORE setsid so the new session is
    // never born holding an inherited /dev/console fd. Without this,
    // illumos may auto-claim /dev/console as the new session's ctty,
    // which then blocks ttymon (in console-login) from claiming it.
    let null_fd = fd.fd_open("/dev/null", fd.O_RDWR(), 0)
    if null_fd >= 0
        fd.fd_dup2(null_fd, 0)
        fd.fd_close(null_fd)
    end if

    process.process_setsid()

    // existing fd 1/2 → log file redirection
    ...
```

Optionally, add an explicit defensive `TIOCNOTTY` after `setsid()` —
open `/dev/tty` (which refers to the current ctty if any) and ioctl
`TIOCNOTTY` to revoke the association. This is a no-op when there's
no ctty, and a fix when one was unexpectedly inherited.

The fd-0-to-/dev/null change alone is the cheaper of the two and
matches the contract most service authors expect: services that need
stdin go open it explicitly.

## Workarounds while pending

1. **Per-service `start.sh` wrapper**: prepend `exec </dev/null` to each
   daemon's exec line. This redirects fd 0 to `/dev/null` from inside
   the service script. Tested on alpha1 — did *not* eliminate the
   ctty grab in our specific syslogd build, suggesting the kernel
   path is more subtle than just fd-0-inherited (or our particular
   syslogd has a separate code path). See companion Hammerhead-side
   fix (TIOCNOTTY in `untty()`) which addressed our concrete symptom.

2. **Reorder so console-login starts before any potential ctty-grabber**:
   change the dependency in `services/hammerhead/console-login.toml` so
   it runs in an earlier tier than syslogd. ttymon claims ctty first;
   later daemons can't steal it.

## Cross-references

- `src/supervisor.reef` — post-fork child setup
- `src/main.reef` — `setup_pid1_console()` (correctly uses `O_NOCTTY`)
- Hammerhead bug filed in parallel: syslogd defensive `TIOCNOTTY` in
  `untty()` to handle the kernel-side symptom from the cmd-side. That
  fix masks the specific syslogd case but doesn't address the broader
  class of any-daemon-may-trip-this — hence this RFE.

## Reporter

Hammerhead build team (Chris Tusa, christusa@gmail.com)
Hit on hh-alpha1 first boot, 2026-05-06.
Recovery: SSH still worked; console login was the only blocked path.
# RFE 002: `type = "group"` service for grouping/targeting

## Summary

Add a `type = "group"` (or `"target"`) service kind to zyginit. A
group service has no `[exec]` block; it exists only as a named
synchronization/enable point. Its `[dependencies]` list expands into
the set of services it represents.

This is the equivalent of systemd's `.target` units. It lets admins
enable/disable a feature set with one command without consolidating
unrelated daemons under a single supervisor.

## Motivation

Hammerhead is converting the NFS, IPsec, and zones SMF service sets
to zyginit. NFS in particular spreads across multiple independent
daemons (`nfsmapid`, `statd`, `lockd`, `nfs4cbd`, plus the `client`
mount oneshot) — each with its own lifecycle, restart policy, and
log file.

The natural design is one zyginit service per daemon. But that loses
the SMF-era ergonomics of "enable nfs-client and the dependents come
along". Without a group abstraction we have two bad options:

1. **Combine multiple daemons into one zyginit service.** Cluttered:
   one contract becomes N contracts, restart semantics get fuzzy,
   log routing needs splitting, and the supervisor's child-death
   handling no longer maps 1:1 to a service.

2. **Document the bundle and tell admins to enable each member.**
   Error-prone and ugly:
   ```
   zygctl enable nfs-cbd nfs-mapid nfs-statd nfs-lockd nfs-client
   ```

A group service neatly avoids both:
```toml
[service]
name = "nfs-client"
description = "NFSv4 client services"
type = "group"
[dependencies]
requires = ["nfs-cbd", "nfs-mapid", "nfs-statd", "nfs-lockd",
            "nfs-client-mount"]
```
Then `zygctl enable nfs-client` is sufficient. `zygctl status nfs-client`
reports composite up/down derived from the members.

## Suggested semantics

- `type = "group"` services have no `[exec]` block. The supervisor
  never forks anything for them.
- Status: `running` iff all `requires` members are `running`;
  `stopped` iff none are; `partial` (or `degraded`) if some.
- `zygctl enable <group>` symlinks the group's TOML in `enabled.d/`
  *and* recursively enables each member that is not already enabled
  (or, simpler: refuses unless all members are also enabled —
  consistent with how SMF dependencies block enable today).
- `zygctl disable <group>` is symmetric.
- `zygctl start <group>` / `stop <group>` operate on the member set.
- Cycle detection treats group-edges the same as service-edges.

## Use cases

| Group | Members |
|---|---|
| `nfs-client` | nfs-cbd, nfs-mapid, nfs-statd, nfs-lockd, nfs-client-mount |
| `nfs-server` | nfsd, mountd, nfs-statd, nfs-lockd |
| `zones` | zones, zonestat |
| `audit` | (just `auditd` after the auditset fold-in — not very useful for audit, but harmless) |
| `network-routing` | forwarding, routing/ndp, network-iptun |

Hammerhead would land this in roughly 4–6 group definitions.

## Reporter

Hammerhead build team (Chris Tusa, christusa@gmail.com)
Filed during SMF-to-zyginit migration policy work, 2026-05-08.
> **CLOSED (2026-07-21):** Fixed and released in **zyginit 0.2.6** (tag
> v0.2.6, rev 221). See the Resolution section below for the two-defect fix.
> Closed by decision ahead of an hh-alpha first-boot confirmation of the
> Defect B (contract reap-race) half — reopen if that boot surfaces a
> regression on the real contract path.

# Bug 003: supervisor exec of service start-commands races first-boot I/O pressure, yielding spurious exit=-1

## Summary

On the **first boot of a freshly-deployed system**, services intermittently
report `exit=-1` in the boot report even though their start command is present,
executable, and succeeds on a manual retry moments later. `exit=-1` here means
the supervisor could not *exec* the start command at all (the process never
ran), as distinct from the command running and exiting non-zero.

The trigger is contention during a reconfigure first boot: `devfsadm` is
enumerating/rebuilding the device tree, the ZFS root pool is finalizing, the
`/reconfigure` flag is being processed, and many services are starting in
parallel. Under that I/O pressure a `stat()`/exec of the start command (or its
`#!` interpreter) can transiently fail, and the supervisor records the service
as failed with `exit=-1`.

Concrete evidence (Hammerhead, hh-alpha9, fresh `hh-deploy create` first boots):

- `acpihpd-check` is a `type="oneshot"` whose `start.sh` **always exits 0**
  (both branches: controller present → `exit 0`; absent → disable acpihpd +
  `exit 0`). Yet on some first boots it shows `exit=-1` and produces **no**
  log output at all — i.e. `start.sh` never executed. A `zygctl restart
  acpihpd-check` seconds later runs it cleanly (`exit=0`, log line written).
- Because `acpihpd-check`'s job is to disable the `acpihpd` daemon on platforms
  without an ACPI hotplug controller, when the check loses the exec race it
  never disables `acpihpd`, so `acpihpd` *also* starts and fails `exit=-1`.
  Result: **two** failed services in the boot report instead of zero, even
  though the intended end state (acpihpd off, correct for i440fx/KVM) is
  reached once things settle.
- The same first-boot `exit=-1` pattern has been observed on other services
  (e.g. `sysconfig`) during boot probes — it is **not acpihpd-specific**; it is
  a general supervisor exec-robustness issue that only manifests under
  first-boot I/O contention.

Note: Hammerhead already mitigates a related symptom by requiring absolute
paths for every external command in service scripts (avoids `PATH`-lookup
misses), but that does not cover this case — here the exec of the start
command / interpreter itself transiently fails despite an absolute, valid path.

## Severity

**Low (cosmetic), but design-defeating.** The eventual end state is correct
and the failure is transient/non-reproducible at steady state, so nothing is
actually broken. However:

- It produces **false-positive "failed" services** in the first-boot report,
  which is alarming and erodes trust in the report.
- It **defeats the `acpihpd-check` oneshot design**: that split (a precheck
  oneshot that disables the daemon so the daemon stays a pure `type="daemon"`
  and never false-positives) was created specifically to avoid `acpihpd`'s
  `exit=-1`, but it only works when the check *wins* the exec race. When the
  check loses, you get exit=-1 on both.

## Affected Version

zyginit 0.2.5 (and earlier — this is longstanding supervisor behavior, not new
to the 0.7.6 migration). Reproduces only on first-boot-of-fresh-deploy under
I/O pressure; does not reproduce at steady state or on subsequent boots.

## Suggested Fix (for the zyginit team)

The supervisor should distinguish "**exec failed to start the process**" from
"process ran and exited with a code," and treat the former as retryable:

1. On a transient exec failure (`ENOENT`/`EACCES`/`EAGAIN`/`ETXTBSY` from the
   spawn), **retry the exec** a few times with a short backoff before recording
   failure. A start command that is genuinely missing will still fail after the
   retries; a transient stat/exec miss will succeed on retry.
2. Alternatively / additionally, **throttle or serialize oneshot exec during a
   reconfigure boot** (while `/reconfigure` is being processed) so the exec
   path isn't competing with devfsadm + pool-finalize I/O.
3. Consider surfacing exec-failure distinctly from process-exit in the boot
   report (e.g. `spawn-failed` vs `exit=N`) so a transient spawn miss is not
   reported identically to a real non-zero exit.

## Reported by

Hammerhead team (Chris Tusa), 2026-07-21. Observed across multiple fresh
`hh-deploy create` first boots on the hh-alpha VMs (pc-i440fx + OVMF, no ACPI
hotplug controller). Cross-ref Hammerhead memory `zyginit-service-empty-path`
(the 2026-06-08 boot probe that first characterized the transient stat() miss).

## Resolution (2026-07-20, zyginit 0.2.6-dev)

Investigation found `exit=-1` was **not** an exec-failure code (a real exec
failure exits **127**); it is the "could not resolve an exit code" sentinel.
Two distinct defects were fixed in `src/supervisor.reef`:

- **Defect A — exec robustness + no silent failure.** The service child now
  retries `process_exec` a bounded number of times (`EXEC_MAX_ATTEMPTS` = 5,
  `EXEC_RETRY_DELAY_MS` = 100ms) to ride out the transient first-boot
  stat()/exec miss. When every attempt fails, the child records the failure to
  **both** its per-service `<name>.log` (via fd 2, which survives a privilege
  drop) **and** the new central `zyginit.log`, then exits 127. This removes the
  "failed with an empty log" symptom. Errno is not exposed by the Reef stdlib
  `process_exec`, so the retry is a blind bounded retry rather than
  errno-selective — a genuinely-missing command still fails all attempts.

- **Defect B — reap-race reconciliation (the actual source of `-1`).** In the
  poll loop the contract-empty event (bundle fd) can beat the child's
  exit-status posting. `handle_contract_event` used to commit the `-1` sentinel
  and tear down tracking, after which the catch-all reaper discarded the real
  code. It now **defers** when there is no hint, no resolvable code yet, and a
  child is still tracked (`rt.pid > 0`), leaving `rt.pid` + the contract map
  intact so the unconditional `reap_children` re-dispatches with the true code
  within ~1s. This fixes both the 127→-1 masking **and** the independent
  false-positive where an exit-0 oneshot that lost the race was reported FAILED.

New central operational log: `/var/log/zyginit/zyginit.log` (`sup_log`),
distinct from the per-service logs. No new report state/label was added
(intentionally — the exit code plumbing now carries the truth).

Verified on Linux (non-contract path) by integration suite
(`tests/integration/run_tests.sh`, Suite 9b "Spawn-failure capture (bug 003)",
97/97 passing): a service pointing at a missing binary now populates both logs
and is reported `failed` (never `exit=-1`). **Defect B rides the Hammerhead
contract path, which the Linux stubs don't exercise — it still needs a fresh
`hh-deploy create` first-boot on an hh-alpha VM to confirm in situ before this
bug is archived.**
> **CLOSED (2026-07-21):** Fixed and released in **zyginit 0.2.7** (tag
> v0.2.7, rev 227). All three suggested fixes landed — see the Resolution
> section below. Non-PID-1 runs now require `--supervisor-test`.

# Bug 004: running the zyginit binary as a non-PID-1 process (e.g. `zyginit -v`) re-runs init and clobbers the live control socket

## Summary

On a system where zyginit is already running as PID 1, invoking the zyginit
binary directly from a shell — e.g. an admin running **`zyginit -v`** to check
the version — does **not** short-circuit and exit. Instead it falls through
into the full supervisor sequence ("non-init mode"), which creates the control
socket. `ctlsocket.create_socket()` **unlinks the existing
`/var/run/zyginit.sock` and binds a new one**, clobbering the socket owned by
the live PID 1. From then on `zygctl` cannot reach the real init:

```
zygctl: cannot connect to zyginit at /var/run/zyginit.sock
zygctl: is zyginit running?
```

The system otherwise keeps running (PID 1 is untouched in memory), but it is no
longer controllable via `zygctl`, and per `main.reef:534` recovery "requires
reboot."

Two distinct defects combine to make this an easy foot-gun:

1. **`-v` is not recognized as a version flag.** Only `--version` and `-V`
   (capital) short-circuit (`main.reef:943`). Lowercase `-v` — the conventional
   "show version" flag for most CLI tools — is unmatched, so `zyginit -v` runs
   the init/supervisor path. An admin's muscle-memory `zyginit -v` is exactly
   the wrong thing.

2. **Non-PID-1 invocation runs the full supervisor with no live-instance
   guard.** The `getpid() != 1` branch is labeled "supervisor mode for testing"
   (`main.reef` ~979/1015), but on a production host it still loads services,
   enters the event loop, and calls the un-guarded socket setup (~`main.reef:1124`,
   not wrapped in `is_pid_1()`), so `create_socket()` clobbers the production
   socket. There is no check for "a live PID-1 zyginit already owns this socket."

## Severity

**Medium.** Not a boot-path defect, but a sharp operational foot-gun: a
completely reasonable admin action (`zyginit -v` to read the version) silently
breaks `zygctl` control of the running system until the next reboot. It is easy
to trigger and non-obvious to diagnose (the socket just "disappears").

## Affected Version

zyginit 0.2.6 (confirmed by source inspection) and 0.2.5 (observed on
Hammerhead hh-alpha9 — an admin ran `zyginit -v` and `zygctl` then reported the
socket unreachable; system was fine but uncontrollable until reboot).

## Suggested Fix (for the zyginit team)

1. **Make `zyginit -v` safe.** Treat `-v` as a version alias (or, more
   conservatively, reject unknown flags with a usage message) so `zyginit -v`
   prints the version and exits **before** any PID-1/supervisor/socket setup —
   the same short-circuit that already exists for `--version`/`-V`.
2. **Guard the socket against a live instance.** Before `create_socket()`
   unlinks/binds `/var/run/zyginit.sock`, detect whether a live PID-1 zyginit
   already owns it — e.g. try to `connect()` first (a successful connect means
   a live server is present → refuse and exit), or check `/proc/1` is zyginit.
   Never unlink a socket that a live server is actively listening on.
3. **Gate "supervisor test mode" behind an explicit opt-in.** Non-PID-1
   supervisor runs (for integration testing) should require an explicit flag
   (e.g. `--supervisor-test`) and/or use a distinct socket path (e.g.
   `$ZYGINIT_SOCK` / a temp path) so a bare `zyginit ...` on a production host
   can never touch the production socket.

## Reported by

Hammerhead team (Chris Tusa), 2026-07-21. Triggered on hh-alpha9 (zyginit 0.2.5,
PID 1) by running `zyginit -v` from a root shell; `zygctl` subsequently could
not connect to `/var/run/zyginit.sock`. Cross-ref Hammerhead memory
`zyginit-no-exec-init-binary` (the known "don't exec the init binary on a
zyginit host — clobbers the socket, recover via `virsh destroy && virsh start`"
note).

## Resolution (2026-07-21, zyginit 0.2.7)

All three suggested fixes implemented (defense in depth):

1. **`-v` / unknown-flag safety** (`src/main.reef`). `-v`, `-V`, and
   `--version` now print the version and exit — **but only when
   `not is_pid_1()`**. This is a deliberately narrower gate than the report
   proposed: the Hammerhead kernel passes `-v` (verbose) as a boot-arg to PID 1
   (`boot-args=-v -m verbose`), so treating `-v` as "print version and exit" in
   the PID-1 case would make init return from `main()` and the kernel would
   re-exec it in a tight loop. Shell invocations (getpid() != 1) short-circuit;
   PID 1 never does.
2. **Live-instance socket guard** (`src/ctlsocket.reef::create_socket`). Before
   unlinking/binding, it `connect()`s the path; if a live server answers it
   logs `REFUSING` and returns -1 instead of clobbering. A stale socket file
   (no listener) fails to connect and is cleaned up as before. `main.reef`
   additionally exits cleanly when a non-PID-1 instance cannot own the socket
   (PID 1 keeps running without a control socket — losing zygctl beats halting
   init).
3. **Opt-in for non-PID-1 supervisor mode** (`src/main.reef`). A non-PID-1 run
   now requires `--supervisor-test`; a bare `zyginit ...` on a host where a real
   zyginit owns PID 1 refuses and exits before any setup. The integration
   harnesses (`run_tests.sh`, `test_shutdown.sh`) and the CLAUDE.md dev
   invocation pass the flag.

Verified by integration suite (`run_tests.sh`, Suite "BUG-004: non-PID-1 socket
safety", 104/104 passing): `zyginit -v` prints the version and creates no
socket; a bare `zyginit` refuses without the flag; and a second instance
launched against a live daemon's socket refuses to clobber it, leaving the
original reachable.