Skip to content

fix: five silent-failure bugs in the CLI/deploy path (SSL renewal, deploy id, server rm, install exit, re-login)#196

Open
Rish-it wants to merge 5 commits into
oblien:mainfrom
Rish-it:fix/critical-deploy-ssl-server-bugs
Open

fix: five silent-failure bugs in the CLI/deploy path (SSL renewal, deploy id, server rm, install exit, re-login)#196
Rish-it wants to merge 5 commits into
oblien:mainfrom
Rish-it:fix/critical-deploy-ssl-server-bugs

Conversation

@Rish-it

@Rish-it Rish-it commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Five independent fixes surfaced while auditing the CLI + its API surface as a
deployment platform. Each is a self-contained commit with a regression test.

1. SSL renewals never retry a failed cert (silent expiry)

Problem. When a renewal throws, the domain is flipped to sslStatus="error".
But both renewal selectors matched only "active"findExpiringSsl
(packages/db/src/repos/domain.repo.ts, the ssl:renew cron) and renewOrgCerts
(apps/api/.../domain.service.ts, the org-wide "renew all"). So once a renew
failed, the domain dropped out of both automatic renewal paths: the cron never
retried it, and its certificate expired unless the operator happened to redeploy
the project or manually re-check SSL (which route through the deploy-time tracker
that can flip it back to active). The automatic safety net — the whole point of
the renewal cron — no longer covered it.

Cause. eq(domain.sslStatus, "active") / d.sslStatus !== "active" — the
error state is exactly the one that most needs retrying.

Fix. Include "error" in both selectors. The cron runs every 6h, well under
Let's Encrypt's failed-validation rate limit. "external" (upstream-managed TLS)
and "provisioning" (no cert issued yet) stay excluded.

2. Deploy trigger returns no deployment_id

Problem. POST /deployments responded { data: { deployment } }, but the CLI
git-deploy path reads data.deployment_id (apps/cli/deploy.ts). The flat id was
undefined, breaking openship deploy --watch and the follow hint. The flat
{ deployment_id, project_id } shape is the established convention elsewhere
(dashboard/lib/api/updates.ts types it; the folder-deploy path already emits
it) — this trigger was the outlier.

Fix. Flatten deployment.id/projectId into the response. Additive —
deployment (and skipped) stay, so readers of those keep working.

3. server rm orphans running containers

Problem. deleteServer hard-deleted a server with no occupancy check,
orphaning the containers of any project deployed to it — server delete does not
enqueue orphaned-resource GC (that's project-teardown only).

Fix. New deployment.countActiveOnServer() counts projects whose current
active
deployment targets the server (matched on the snapshot meta->>'serverId').
deleteServer returns 409 when that count is non-zero, unless ?force=true.
The CLI rm gains a confirmation prompt and --force (which also forwards
?force=true).

4. Non-streaming install exits 0 on failure

Problem. openship server install (without --follow) exited 0 even when a
component failed, unlike the streaming path — CI read a partial install as
success.

Fix. Track failures and exit(1) when any component fails.

5. Re-login resets a context's endpoints to localhost

Problem. --api-url/--dashboard-url carried localhost defaults, so
commander set them even when unspecified. Re-authing an existing context without
those flags overwrote its saved endpoints with localhost, silently repointing a
prod/staging context.

Fix. Drop the option defaults; resolve flag → saved context → localhost. An
omitted flag keeps the stored URL; an explicit flag still overrides.

Tests

Regression test per fix, each proven RED before the fix and GREEN after:

  • domain.repo.test.tsfindExpiringSsl selects active + error, excludes
    external/provisioning/fresh (PGlite).
  • deployment.controller.test.ts — response carries deployment_id/project_id.
  • deployment.repo.test.tscountActiveOnServer counts only active-pointer
    deployments (PGlite); servers.controller.test.ts — 409 when occupied, bypass
    on force, delete when free.
  • server.test.ts — install exits 1 on component failure; rm --force sends
    ?force=true.
  • login.test.ts — re-login without --api-url keeps the saved endpoint; an
    explicit flag overrides.

Full suites green: packages/db (34), apps/cli (81), apps/api (749); tsc --noEmit clean in all three.

Not included

A sixth candidate — a preview deploy overwriting the production active-deployment
pointer — is a genuine bug but its fix depends on the intended environment model
(per-env project rows vs. a per-deployment environment flag), which isn't clear
from the code. Filed as #195 (a question) rather than guessing in the
active-deployment path.

Rish-it added 5 commits July 24, 2026 12:38
A renewal that throws flips the domain to sslStatus="error", but both
renewal selectors matched only "active": findExpiringSsl (the ssl:renew
cron) and renewOrgCerts (the org-wide "renew all"). Once a renew failed,
the domain was never picked up again by either path, so its certificate
expired silently with no further attempt.

Include "error" in both selectors so a failed renewal is retried on the
next sweep (every 6h; well under Let's Encrypt's failed-validation rate
limit). "external" (upstream-managed TLS) and "provisioning" (no cert
issued yet) stay excluded.
POST /deployments responded { data: { deployment } }, but the CLI git-deploy
path reads data.deployment_id (apps/cli deploy.ts). The flat id was undefined,
breaking `openship deploy --watch` and the follow hint. The flat
{ deployment_id, project_id } shape is the convention elsewhere (dashboard
lib/api/updates.ts types it; the folder-deploy path already emits it) — this
trigger was the outlier.

Flatten deployment.id/projectId into the response. Additive: `deployment` (and
`skipped`) stay so existing readers of those keep working.
deleteServer hard-deleted a server with no occupancy check, orphaning the
running containers of any project deployed to it — server delete does not
enqueue orphaned-resource GC (that path is project-teardown only).

Add deployment.countActiveOnServer(): projects whose CURRENT active
deployment targets the server, matched on the snapshot meta serverId.
deleteServer now returns 409 when the count is non-zero unless
?force=true is passed.
Two robustness gaps in the `server` command:

- `rm` deleted with no confirmation; it now prompts (y/N) interactively
  and takes --force to skip. --force also forwards ?force=true so the
  API's active-deployment guard is bypassed for a deliberate teardown.
- Non-streaming `install` exited 0 even when a component failed, unlike
  the --follow path. It now exits 1 when any component fails, so CI and
  scripts don't read a partial install as success.
The --api-url/--dashboard-url options carried localhost defaults, so
commander set them even when unspecified. Re-authing an existing context
without those flags wrote the localhost defaults over its saved
endpoints, silently repointing a prod/staging context at localhost.

Drop the option defaults and resolve from flag -> saved context ->
localhost, so an omitted flag keeps the stored URL and an explicit flag
still overrides.
Copilot AI review requested due to automatic review settings July 24, 2026 07:56

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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.

2 participants