Skip to content

Discuss alternatives for getRelayUptimeSeconds error handling #590

Closed

Description

@CKodidela

Context

In #585 , getRelayUptimeSeconds was patched to wrap
runCommandWithOutput in a try/catch to handle the case where
Docker is not installed. When Docker is missing, spawn emits
an ENOENT error instead of returning a non-zero exit code, which
was causing info --json to crash and break the pre-commit hook.

Problem

The current fix works but is a short-term solution. We should
discuss better long-term alternatives.

Potential Alternatives

  • Pre-check if Docker is installed before invoking the command
  • TBD

Goal

Agree on a consistent approach so similar issues don't arise in
other command wrappers.

Activity

  1. CKodidela commented on Apr 28, 2026

    @CKodidela
    ContributorAuthor
  2. CKodidela commented on Apr 29, 2026

    @CKodidela
    ContributorAuthor

    @cameri we can discuss on this issue when you are available

  3. cameri commented on Apr 29, 2026

    @cameri
    Owner

    @Mahmoud-s-Khedr since you worked on the CLI migration, do you mind providing your recommendation here?

  4. Mahmoud-s-Khedr commented on Apr 29, 2026

    @Mahmoud-s-Khedr
    Contributor

    HI, @cameri @CKodidela, my recommendation is to solve this in the shared command runner (the layer behind runCommandWithOutput) instead of per call site try/catch.

    spawn errors like ENOENT (docker not installed) are a different failure path from non-zero exit codes, and today callers need to know that detail. That’s why info --json crashed.

    I’d suggest we standardize runCommandWithOutput to return a normalized result/error model for all cases:

    • command not found (ENOENT)
    • permission denied (EACCES)
    • non-zero exit
    • timeout
    • terminated by signal

    Then callers only apply feature policy:

    • optional dependency: degrade gracefully (e.g. null / unknown)
    • required dependency: fail with a clear actionable error

    This gives us one consistent contract across the CLI and prevents repeating the same class of bug in other wrappers.

    What do you think do you this approach?

  5. CKodidela commented on Apr 30, 2026

    @CKodidela
    ContributorAuthor

    Agreed. Normalizing all failure cases in runCommandWithOutput keeps callers clean and prevents the same bug from recurring elsewhere. A discriminated union on the return type would make the contract explicit.

  6. CKodidela commented on Apr 30, 2026

    @CKodidela
    ContributorAuthor

    @cameri i can make a PR if this suggestion is agreed upon

  7. cameri commented on Apr 30, 2026

    @cameri
    Owner

    Yes, let's move forward with @Mahmoud-s-Khedr recommendation

  8. CKodidela commented on Apr 30, 2026

    @CKodidela
    ContributorAuthor

    Should I work on this?
    If yes can you assign it.

  9. cameri commented on Apr 30, 2026

    @cameri
    Owner

    Assigned to you

  10. Mahmoud-s-Khedr commented on Apr 30, 2026

    @Mahmoud-s-Khedr
    Contributor

    @CKodidela feel free to tag me if you need any help with this.

  11. CKodidela commented on Apr 30, 2026

    @CKodidela
    ContributorAuthor

    Sure

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

Metadata

Metadata

Assignees

Labels

No labels
No labels

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions