Skip to content

ci: make the integration suite pass again - #6767

Merged
otavio merged 5 commits into
masterfrom
fix/ssh-tests-timeout
Jul 30, 2026
Merged

ci: make the integration suite pass again#6767
otavio merged 5 commits into
masterfrom
fix/ssh-tests-timeout

Conversation

@otavio

@otavio otavio commented Jul 30, 2026

Copy link
Copy Markdown
Member

What

Makes validate (tests - postgres) pass again, and cuts it from ~50 minutes to ~10.

The job had two stacked faults: the suite did not fit its timeout, and the timeout was hiding
the fact that every SSH case was failing outright.

Why

The timeout

TestSSH runs ~31 cases against both transport versions, each standing up its own agent
container, and did not fit in the 25m the unit-test projects share:

panic: test timed out after 25m0s
	running tests:
		TestSSH (25m0s)
		TestSSH/connection_v2 (10m59s)

What hid this is the paths filter. The job only reports success when it skips the Go tests:
every green run lasts about a minute (e.g. run 30501644237, on a commit touching only
.github/workflows/notify-cloud.yml). Every run that actually executed the suite timed out.
The job has been failing whenever it does any work, and passing only when it does none.

What the timeout was hiding

Given a longer budget the suite completes — and reports all 56 SSH cases failing:

ssh: handshake failed: ssh: unable to authenticate, attempted methods [], no supported methods remain

77ce019a0 feat(api): persist access policies, identities, approvals and accounts made
namespaces identity-first by default, both at the store seam
(server/api/store/pg/namespace.go) and in the schema
(ssh_access_mode text not null default 'identity').

The suite creates its namespace through cli namespace create, which had no way to ask for
another mode, so the namespace was born in identity mode. There,
newServerConfigCallback returns a PartialSuccessError advertising publickey only — so
the suite's password logins never get to try a password, and its ad-hoc keys are not enrolled
identities either.

Confirmed on a live stack, changing nothing but the DB column:

ssh_access_mode = identity  →  Authentications that can continue: publickey
ssh_access_mode = legacy    →  Authentications that can continue: password,publickey

And by bisect: 77ce019a0 is absent from 4eca6cbcc (the last green run of this job) and
present in c7c3cdf4f (the first failing one). Every green run since has been a skip.

Why it was so slow

Each of the 62 cases builds an agent container. With KeepImage: false testcontainers deletes
the image after every case, so the next build re-exports every layer even though the build
cache is warm — you can see CACHED on every Dockerfile step while the case still costs ~47s.
That was ~40s of pure re-export per case, roughly 40 minutes across the run. The same applies
to the cli image, built four times per run.

Changes

  • .github/workflows/qa.yml: -timeout comes from an optional test_timeout matrix key,
    45m for the tests project, defaulting to 25m elsewhere. With the perf fixes below the
    suite lands around 10m, so this is now headroom rather than the fix.
  • cli: namespace create grows --ssh-access-mode legacy|identity. Namespaces are born
    identity-first and the API refuses a switch back to legacy unless ssh_legacy_allowed is
    set, so before this there was no way to provision a legacy namespace. Operators migrating an
    existing fleet need one too, not just the tests.
  • tests/ssh_test.go: TestSSH now asks for legacy explicitly, keeping it a test of that
    model. New TestSSHIdentityMode covers the identity one: password is never offered, and a
    key enrolled as an SSH identity connects. Access mode is orthogonal to the transport, so it
    runs on the default version rather than doubling the table.
  • tests/ssh_test.go / tests/environment/docker_compose.go: KeepImage: true for the
    agent and cli images. The image is still rebuilt from the current context on every case, so a
    change to the agent source is picked up exactly as before; only the discard-and-re-export is
    gone.
  • tests/ssh_test.go: agent containers are terminated per case instead of accumulating on
    the parent test for the whole run. Setup is factored into newSSHEnvironment and
    startAcceptedAgent.
  • tests/environment/configurator.go: on a compose bring-up failure, assert.FailNow
    called err.Error() on the err from NewDockerComposeWith — which is nil once that call
    succeeded. A bring-up failure therefore died with a SIGSEGV that masked the real cause. It
    now reports the error from Up.

Testing

validate (tests - postgres) passes in 10m34s, against a 48m timeout failure before.

Locally, all 62 cases green at each step, isolating the two perf changes:

wall clock
before 3109.9s — 51.8 min
+ agent KeepImage 621.1s — 10.4 min
+ cli KeepImage 547.3s — 9.1 min

Per case, same five cases measured back to back and warm: ~47s → ~6.2s.

Review notes

--ssh-access-mode lets an operator create a legacy namespace without ssh_legacy_allowed
being set. That looks right — the flag is on namespace creation, the guard is on user-facing
mode switching, and the CLI is an admin tool that can already create users and namespaces —
but it is a deliberate escape hatch and worth a second opinion.

cloud / validate-server-enterprise fails on this branch, and not because of it: this PR
touches only qa.yml, cli/** and tests/**, none of which cloud imports. It is
shellhub↔cloud drift on master — a services.Service interface mismatch from c496c1026,
plus unresolved FirewallConnection / WebEndpoint / ErrFirewallBlocked /
RegisterFirewallEvaluator symbols. An unrelated sync-deps dispatch is failing too. The
check passed on this PR's original base and appeared only when it was rebased onto master, so
it needs fixing in cloud, separately.

Follow-up, not in this PR

The suite still runs the whole table for both transport versions. Now that a case costs ~6s
rather than ~47s, running the full table under both access modes is affordable in a way it was
not before, if that coverage is wanted.

@otavio
otavio requested a review from a team as a code owner July 30, 2026 14:48
@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown

Claude finished @otavio's task in 59s —— View job


Code Review Complete

Reviewed 2 files across code quality, security, testing, language patterns, and architecture — no new issues found. The code looks good as-is.

Notes for context (not blocking):

  • tests/environment/configurator.go:129 — The fix is correct. The old code passed the Up result inline to assert.NoError but then called err.Error() on the outer err from NewDockerComposeWith (line 103), which is nil after that call succeeds — a real nil-dereference on the failure path. Scoping err to the if statement resolves it cleanly, and it doesn't shadow the outer err (still used at lines 104–105) or clash with the err := on line 134.
  • .github/workflows/qa.yml:121${{ matrix.test_timeout || '25m' }} is valid Actions expression syntax; the undefined matrix key evaluates falsy for the other projects, so they keep the 25m default while tests gets 45m. The paired tests/ change is what makes CI actually exercise the suite under the new budget, matching the PR's stated intent.

To request another review round, comment /review.

otavio added 3 commits July 30, 2026 13:02
The tests project shares the 25m budget with the unit-test projects, but it does far
more work: TestSSH runs ~31 cases against both transport versions, and each case builds
and starts its own agent container, waits for the device to register, accepts it and
waits for it to come online. The CI dumps show it needs roughly 28m -- at the 25m mark
connection_v2 was 11m in and still progressing, so nothing is hung; the suite simply
does not fit.

This job only ever reported success when the paths filter skipped the Go tests
altogether: every run of ~1m is a skip, and every run that actually executed the suite
timed out. Give the integration project its own 45m budget and leave the unit-test
projects on 25m.

Also fix the compose bring-up failure path, which reported the wrong error: it called
Error() on the err from NewDockerComposeWith, which is nil once that call has succeeded,
so a bring-up failure died with a nil dereference that hid the real cause instead of
reporting it.
Namespaces are born identity-first at the store seam, and the API refuses a switch back to
legacy unless ssh_legacy_allowed is set, so there was no way to provision a legacy namespace
at all. Operators migrating an existing fleet — and the integration suite, which authenticates
with passwords — need one.
Every TestSSH case has failed since namespaces became identity-first: the suite's namespace
was born in identity mode, where the server advertises publickey only, so password logins got
"attempted methods []" and ad-hoc keys were not identities. The 25m timeout masked this — the
run died before reporting.

TestSSH now asks for legacy explicitly, keeping it a test of that model, and TestSSHIdentityMode
covers the identity one: password is never offered, and an enrolled key connects. The access
mode is orthogonal to the transport, so it runs on the default version rather than doubling the
table.

Agent containers are now terminated per case instead of accumulating on the parent test.
@otavio
otavio force-pushed the fix/ssh-tests-timeout branch from bf9d66c to 7068b18 Compare July 30, 2026 17:10
@otavio otavio changed the title ci: give the integration suite a timeout it can finish in ci: make the integration suite pass again Jul 30, 2026
otavio added 2 commits July 30, 2026 15:13
Each of the 62 cases builds its own agent container. With KeepImage false testcontainers
deletes the image after each one, so the next build re-exports every layer even though the
build cache is warm. Measured locally on the same five cases: ~47s each before, ~6.2s after.

The image is still rebuilt from the current context on every case, so a change to the agent
source is picked up as before; only the discard-and-re-export is gone.
Same discard-and-re-export cost as the agent image, at a much smaller scale: the cli image is
built four times per run, once for each user and namespace the two environments create.
Measured locally on the same five cases, back to back and warm: 176.0s before, 135.3s after.
@otavio
otavio merged commit 04e760a into master Jul 30, 2026
41 of 42 checks passed
@otavio
otavio deleted the fix/ssh-tests-timeout branch July 30, 2026 18:37
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.

1 participant