Skip to content

Harden the restart-test PID lookup: read hyperd's pid file instead of lsof, and name the expired readiness phase #311

Description

@StefanSteiner

Context

The Linux restart-test failure on #304 was root-caused to lsof ORing its list-selection options: lsof -nP -t -c hyperd <socket path> returned every hyperd on the runner instead of the socket's owner, so the test SIGKILLed a different test's engine. Full analysis in #310.

#304 shipped the minimal fix (add -a so lsof ANDs its selectors), which is verified correct and green on all platforms. A second, independent investigation produced a more robust version that is worth folding in.

What to fold in

Branch fix/uds-restart-linux-opus, head 1e5b78d (based on e51fdefb, #304's pre-fix head — it will need rebasing onto current main, since #304's -a fix now occupies the same function).

1. Read hyperd's own PID lock file instead of shelling out to lsof.
Display for ConnectionEndpoint::DomainSocket renders "{directory}/{name}", and hyperd's PIDFile is directory + "/" + name + ".pid" — so <endpoint>.pid is exactly the authoritative file, under both the hyper and .s.PGSQL.<port> naming modes. Identity is then confirmed with ps -o comm= before the PID is returned (a stale file can name a recycled PID).

Why this is better than -a: it removes the OR/AND footgun entirely and drops the external-tool dependency for the socket path, reading the same source hyperd uses for its own cross-instance locking. -a is correct but leaves a sharp edge for the next person editing that helper.

2. Make the readiness helper name which phase expired.
wait_for_live_hyperd_after_kill currently fails with a bare "daemon should restart hyperd within 45s". That opacity is the reason this bug consumed two CI runs and two independent investigations — the message told us nothing about whether the old endpoint never died or the new one never came up. Returning an error that names the expired phase would have pointed straight at the mis-targeted kill.

This is the higher-value half of the change.

Verification already done

On 1e5b78d: three consecutive fully-green CI runs (34099539806, 34100732250, 34101708139), with the restart tests green in 6 of 6 ubuntu runs and completing in ~5-6s instead of burning the 45s budget.

Notes

  • Test-only; no production code changes, so no changelog entry is required (AGENTS.md reminder 8).
  • Optional and explicitly not recommended without new evidence: giving each spawned engine a fresh socket path per restart. After the fix, the SIGKILL→rebind-same-path route is genuinely exercised on Linux for the first time and passed 6/6, so that would be speculative complexity. Revisit only if a run ever shows a spawn failure with "domain socket is in use" — and the phase-naming errors above would say so explicitly rather than timing out silently.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions