mirror of
https://github.com/coder/coder.git
synced 2026-10-29 17:44:33 +08:00
2574e6b7852182aeaf47784e91d5e136b68e3fa7
9
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 J. Scott Miller
|
1dea00dd04 |
fix: deflake TestWorkspaceTagsTerraform with context-aware build waits (#26315)
`TestWorkspaceTagsTerraform` runs a real terraform provisioner but waited on builds with `coderdtest` helpers whose deadlines are sized for the echo provisioner used by most tests, which replays canned responses and completes in well under a second. On Windows runners, where terraform providers are not cached and every `terraform init` downloads from the registry, template imports exceeded the 25s budget in `AwaitTemplateVersionJobCompleted` and workspace builds exceeded the 10s context in `AwaitWorkspaceBuildJobCompleted`, even though the test intends a 120s budget. Add `AwaitTemplateVersionJobCompletedWithTimeout` and `AwaitWorkspaceBuildJobCompletedWithTimeout`, which take a caller-provided wait bound, and use them in the test with `2*testutil.WaitSuperLong` (120s). Also fix `AwaitWorkspaceBuildJobCompleted` creating a `WaitShort` (10s) context while polling for `WaitMedium` (15s), which guaranteed `context deadline exceeded` errors for the final five seconds of polling. `TestWorkspaceTemplateParamsChange` has the same shape (real terraform provisioner, 120s test context, plain await helpers) and the same latent bug, so it gets the same fix. Closes https://github.com/coder/internal/issues/1470 (Linear: PLAT-176) <details> <summary>Root cause analysis</summary> Two CI failures, same mechanism: - 2026-04-16 (run 24493089585, windows-2022): `overrides_with_dynamic_option_from_var/dynamic` failed at `coderdtest.AwaitTemplateVersionJobCompleted` with `Condition never satisfied ... make sure you set IncludeProvisionerDaemon!`. The template import job (real terraform init/plan, with network provider download) did not complete within `WaitLong` (25s). - 2026-05-27 (run 26492817796, windows-2022): `tag_param/dynamic` failed at `coderdtest.AwaitWorkspaceBuildJobCompleted` with `failed to get workspace build ...: context deadline exceeded`. The helper's internal context was `WaitShort` (10s) while its polling window was `WaitMedium` (15s), so after 10s every poll could only fail. The logged `terraform apply: exit status 1` and the `TempDir RemoveAll ... Access is denied` cleanup error are consequences of test teardown canceling the in-flight job while the provider exe was still file-locked. The test declares a 120s budget (`2*testutil.WaitSuperLong`, commented "This can take a while"), but the await helpers ignored it and applied their own 10-25s budgets. `testutil.CacheTFProviders` is a no-op on Windows, so real builds are much slower there. This change raises the ceiling for the tests rather than making terraform faster; both observed failure signatures are eliminated. The default helper budgets are unchanged for the ~880 existing call sites. One small behavior change: `AwaitTemplateVersionJobCompleted` previously marked the test failed on any transient poll error via `assert.NoError`; it now logs and keeps polling, matching the workspace build helper, and still fails on timeout. `TestWorkspaceTemplateParamsChange` is the sibling real-terraform test in the same file (also covered by the original provider-caching work in #20603). It runs three sequential real builds with the plain await helpers under a 120s context, so it is exposed to the same Windows slowness even though it has not produced its own issue yet. Its context is raised to `6*testutil.WaitSuperLong` to outlast three sequential await budgets. API note: a context-taking variant was considered first, but a `time.Duration` parameter avoids an implicit "context must have a deadline" contract and matches how the existing helpers manage their own wait budgets. </details> --- 🤖 This PR was generated by Coder Agents on behalf of @jscottmiller. |
||
|
J. Scott Miller
|
fd83c35531 |
test(provisionerd): standardize cancel-tolerant acquire asserts (#26386)
`TestProvisionerd/CloseCancelsJob` closes the daemon inside the
provisioner parse/init callback, which cancels the in-flight
`AcquireJobWithCancel` RPC immediately after the job is delivered. dRPC
can report `context.Canceled` from the server-side `stream.Send` even
after the message was successfully sent, causing a spurious
`assert.NoError` failure.
The tolerance for this race already existed in the `acquireOne` test
helper (added in #17448 for coder/internal#584), but the inline acquire
handlers each carried their own bare `assert.NoError`, so
`CloseCancelsJob` never got it and flaked.
This extracts a single `assertAcquireNoError` helper that tolerates
`context.Canceled` on an acquire-stream `Send`/`Recv` (and fails on any
other error), and routes every `AcquireJobWithCancel` test handler and
`acquireOne` through it. The tolerance now has one definition and cannot
drift out of sync. If a job is never delivered, the affected tests still
fail waiting on their completion channels.
Fixes PLAT-172. Refs https://github.com/coder/internal/issues/1478
<details>
<summary>Investigation and validation</summary>
- The reported failure was `assert.NoError` at
`provisionerd_test.go:112` (commit
|
||
|
J. Scott Miller
|
d8b76831ff |
test(testutil): retry terraform provider cache population on transient failures (#26196)
## Problem
Tests that run real Terraform (e.g.
`enterprise/coderd.TestWorkspaceTemplateParamsChange`,
`provisioner/terraform.TestProvision`) intermittently fail when
populating the shared provider cache. On a cache miss,
`DownloadTFProviders` shells out to `terraform init` and `terraform
providers mirror` against the live registry, which periodically returns
transient 5xx errors from the registry/GitHub (504, 500). The
cache-population helper had no application-level retry, so a single
transient failure failed the whole test. Terraform's own registry client
only retries each request once ("the request failed after 2 attempts"),
which is insufficient for these bursts.
## Fix
`runCmd` now retries on any non-zero exit using
`github.com/coder/retry`, logging each failed attempt and preserving the
original failure message format. Retry-all is safe here because
`terraform init` and `terraform providers mirror` are idempotent: each
run reconciles the existing state in the working directory.
The backoff window is deliberately wide: 5 attempts with a 5s floor and
30s ceiling. `coder/retry` grows the delay by phi (~1.618) from the
floor and caps it at the ceiling, so attempts start at roughly t=0s, 8s,
21s, 42s, and 72s (waits of ~8.1s, ~13.1s, ~21.2s, and 30s capped, plus
command runtime). Registry/GitHub incidents typically last seconds to
minutes rather than a single unlucky request, so a narrow window would
only survive an isolated blip, while the early second attempt (~8s)
still recovers quickly from brief ones. This is affordable because the
network path runs only on a cache miss, not on every test: a populated
cache short-circuits via `os.Stat` and is reused within and across runs
(persisted by `.github/actions/test-cache`). The wait is therefore
rarely incurred and is negligible against the 20m per-package test
timeout. The only downside is a slower failure on a genuinely doomed
run.
This only affects the test provider-cache helper. Production provisioner
code, the Windows no-op path, and the CI cache strategy are unchanged.
Refs https://github.com/coder/internal/issues/1201
<details>
<summary>Investigation notes</summary>
The CI cache (`~/.cache/coderv2-test`, via `.github/actions/test-cache`)
is persisted across runs and keyed by a hash of a stable caller-supplied
label + template file contents, so cache hits avoid the network
entirely. The flake only surfaces on a cache miss (provider version
bump, monthly cache reset, or new label/template), where the populating
`terraform init` was the sole unprotected network call. This change
closes that gap without weakening the "use real Terraform" intent of the
tests.
</details>
---
🤖 Generated with Coder Agents on behalf of @jscottmiller.
|
||
|
J. Scott Miller
|
3c5160dd66 |
test(scaletest/workspacetraffic): fix RPTY close flake on graceful timeout (#26199)
## Summary Fixes the `TestRun/RPTY` flake tracked in PLAT-116 (`timeout waiting for read to finish`). `rptyConn.Close` sends `Ctrl+C` to interrupt the command, then waits up to 30s for the read to finish. The read only unblocks once the server closes the reconnecting PTY stream, which depends on the agent terminating the command under test (a `dd` reading stdin) and tearing down the backend. When the server-side teardown does not complete within 30s, `Close` returned a hard error and failed the run. Logs from the March 2026 failure confirm the agent used the `screen` backend (`backend_type=screen`) and show no session teardown activity at all after `Ctrl+C`; the interrupt chain stalled rather than merely running slowly. The previously deferred `c.conn.Close()` ran only *after* the wait gave up, so nothing actively unblocked the read within the window. ## Changes - `conn.go`: graceful close is now best-effort. After the grace period, `Close` actively force-closes the underlying connection to unblock the read, waits a bounded `forceCloseReadTimeout` (5s) for the read to drain rather than blocking indefinitely, and returns a distinguishable sentinel `errRPTYGracefulCloseTimeout`. The same force-close path is used when the `Ctrl+C` write fails. Timeouts are fields on `rptyConn` so tests can shrink them deterministically. - `run.go`: treats `errRPTYGracefulCloseTimeout` as non-fatal (logged as a warning) so the run no longer fails when the connection was closed, just not gracefully. Any other close error still fails the run, preserving signal for a genuine regression. - `conn_internal_test.go`: new unit tests covering the graceful, forced-close, stuck-read-after-close, and double-close paths using a stub connection. ## Testing - `go test ./scaletest/workspacetraffic/ -run TestRPTYConnClose -race -count=10` passes. - `go test ./scaletest/workspacetraffic/ -run TestRun/RPTY` passes. - `golangci-lint run ./scaletest/workspacetraffic/` clean; `gofmt`/emdash clean. <details> <summary>Root-cause analysis and lifecycle notes</summary> The client conn is bound to `context.Background()`, so the test context cannot unblock the read; only an actual websocket close can. The coderd proxy bridges client and agent with `agentssh.Bicopy`, which propagates closes promptly, so the stall is not there. On the agent side both backends do eventually close the connection after the command exits: - **buffered**: output reader hits EOF on command exit and closes active conns in-process (one goroutine handoff). - **screen**: a longer chain (`Ctrl+C` -> screen client PTY -> daemon -> inner PTY -> SIGINT -> `dd` exit -> session teardown -> `screen -x` client exit -> agent output reader EOF -> conn close), involving extra OS processes. The backend is auto-selected (`screen` if present on Linux, else `buffered`) and the test does not pin it, so behavior depends on the runner image. Logs from the March 2026 failure (run 23322663002) confirm `backend_type=screen` and show no `unable to read pty output` or session-quit activity between the attach and the moment the client gave up 30s later, meaning `dd` never exited in response to `Ctrl+C` within the window. The stall is in delivery or signal handling inside the screen path, not a slow process exit. No agent-side logic bug was identified from the logs, which is why the fix makes graceful close best-effort rather than asserting a fixed deadline. Possible follow-ups (not in this PR): pin the test to a deterministic backend, and/or log the agent's chosen `backend_type` in test output to aid future diagnosis. </details> --- This PR was generated with assistance from Coder Agents. |
||
|
J. Scott Miller
|
3da8226876 |
fix(coderd/externalauth): support disabling token refresh retries (#26231)
`TestRefreshToken/RefreshRetries` flakes on Windows. The subtest disables transient-failure refresh retries by setting `RefreshRetryTimeout = time.Nanosecond`, but a near-zero timeout cannot deterministically prevent a retry: on coarse-clock platforms the 1ns deadline may not register as expired until after the first refresh attempt completes, and `retry.Wait`'s first delay is zero, so an extra IDP refresh attempt slips through and the attempt-count assertion fails with `refreshCount = totalRefreshes + 1`. A negative `RefreshRetryTimeout` now disables transient-failure retries explicitly so exactly one refresh attempt is made, and the test sets `-1` instead of `time.Nanosecond`. The retry config fields are only set from tests, so default refresh behavior is unchanged. Closes https://github.com/coder/internal/issues/1550 (PLAT-293) <details> <summary>Root cause analysis</summary> 1. The test sets `RefreshRetryTimeout = time.Nanosecond` intending "no retries". 2. `refreshTokenWithRetry` creates `context.WithTimeout(ctx, 1ns)`. On Linux this context is canceled synchronously at creation: consecutive `time.Now()` reads differ by more than 1ns, so `context.WithDeadline` observes `time.Until(deadline) <= 0`. The `retryCtx.Err() != nil` guard then deterministically stops after one attempt. 3. On Windows, `time.Now()` is coarse, so both clock reads inside `WithTimeout` can return the same instant, and a real 1ns timer is scheduled instead of synchronous cancellation. 4. The fake IDP is served in-process, so the first refresh attempt can complete before that timer fires. `retryCtx.Err()` is still nil and `retry.Wait`'s first delay is zero, so a second refresh attempt happens. 5. `require.Equal(t, refreshCount, totalRefreshes)` then fails with `expected: 2, actual: 1` (or `4 vs 3` when the race hits a later loop iteration), matching all CI occurrences. Timing-based test-side mitigations cannot close this race, so the fix adds explicit retry-disable semantics instead. `RefreshRetries` passed 100 consecutive local runs with the change. </details> *This PR was generated by Coder Agents on behalf of @jscottmiller.* |
||
|
J. Scott Miller
|
381c4202d2 |
fix(coderd): deflake TestWorkspaceBuildStatus audit log assertion (#26232)
Fixes a flake in `TestWorkspaceBuildStatus` where the test asserted an exact audit log count immediately after the stop build completed: ``` workspacebuilds_test.go:1261: Error: "[...]" should have 7 item(s), but has 6 ``` The audit log for a workspace build is exported asynchronously relative to what `AwaitWorkspaceBuildJobCompleted` observes, so the strict `require.Len` could run before the stop log was recorded. The assertion now polls with `require.Eventually` until the expected log count and stop action appear, matching the existing poll pattern in the file. Also fixes the same race in `TestWorkspaceDormant/StartWakesUpDormantWorkspace` (`workspaces_test.go`), flagged during review as a sibling risk: its exact `require.Len(t, auditor.AuditLogs(), 2)` after build completion is now an equivalent `require.Eventually` poll. Verified with `go test ./coderd -run TestWorkspaceBuildStatus -count=10` and `go test ./coderd -run 'TestWorkspaceDormant/StartWakesUpDormantWorkspace' -count=5`. Closes https://github.com/coder/internal/issues/1565 (PLAT-304). 🤖 Generated by Coder Agents on behalf of @jscottmiller |
||
|
J. Scott Miller
|
1ed5c6f9df |
fix(dogfood/coder): install libx11-xcb1 for desktop Chrome (#26149)
## Problem
Launching the system Google Chrome from the portabledesktop/VNC session
in dogfood workspaces crashes immediately (SIGTRAP, exit 133). Headless
Chrome and Playwright's bundled Chromium are unaffected, so it only
shows up when a user opens Chrome in the desktop.
Root cause traced via `strace`: Chrome `dlopen`s `libX11-xcb.so.1` when
it initializes the X11 GUI path and `IMMEDIATE_CRASH()`es when the
library is missing:
```
openat(... "/usr/lib/x86_64-linux-gnu/libX11-xcb.so.1" ...) = -1 ENOENT
... --- SIGTRAP {si_signo=SIGTRAP, si_code=SI_KERNEL} ---
```
The Google Chrome `.deb` declares `libx11-6` and `libxcb1` but **not**
`libx11-xcb1`, so apt never installs it.
## Why it regressed (~2 weeks ago)
`libx11-xcb1` used to be pulled in transitively by the build-time step:
```
pnpm dlx playwright@1.47.0 install --with-deps chromium
```
`--with-deps` makes Playwright `apt-get install` Chromium's full
system-library set, which on Ubuntu includes `libx11-xcb1`.
#25448 ("refactor: build dogfood image as base + mise oci layers")
removed that build step from both Ubuntu Dockerfiles and moved the
install to the workspace-start `install-deps` script, which runs
`playwright install chromium` / `playwright-core install --no-shell
chromium` **without** `--with-deps`. Those download only the browser
binaries, not the apt system libraries, so `libx11-xcb1` disappeared
from the image.
## Fix
Install `libx11-xcb1` explicitly in both `ubuntu-22.04` and
`ubuntu-26.04` `Dockerfile.base` apt lists, decoupling desktop Chrome's
X libraries from the Playwright install so it can't silently regress
again.
## Testing
Reproduced and verified on a running dogfood workspace (Ubuntu 22.04):
before the fix, `DISPLAY=:1 google-chrome` crashed with SIGTRAP on the
missing `libX11-xcb.so.1`; after `apt-get install -y libx11-xcb1`, a
vanilla `google-chrome` launch (as the desktop menu invokes it) starts
and stays running with a renderer process. Remaining EGL/GPU warnings
are expected (software rendering under VNC) and non-fatal.
<details>
<summary>Investigation notes</summary>
- headless Chrome and `--screenshot` worked, isolating the failure to
the X11/Ozone GUI path.
- The earlier `crashpad ... format error / Unknown ptrace scope`
messages were a red herring; the real crash was the missing-library
`dlopen`.
- `portabledesktop` ships a self-contained Xvnc server + WM runtime (its
own glibc closure) and does not provide libraries to system apps like
Chrome, so the fix belongs in the dogfood image.
</details>
---
Generated with Coder Agents.
|
||
|
J. Scott Miller
|
3e46c7986f |
feat: event driven agent connection metric (#24355)
Moves the `coderd_agents_first_connection_seconds` histogram from the polling-based `prometheusmetrics.Agents()` loop to the event-driven `agentConnectionMonitor.init()` path. The metric is now recorded exactly once when an agent first connects over the RPC websocket, instead of being retroactively computed each polling tick. The `username` and `workspace_name` labels are removed to reduce cardinality; only `template_name` and `agent_name` are retained. Adds unit tests covering both the happy path (first connection recorded) and the negative-duration guard (clock skew logs a warning, no sample emitted). |
||
|
J. Scott Miller
|
20b953a99d |
feat: add Prometheus metric for agent first connection duration (#24179)
## Summary Add `coderd_agents_first_connection_seconds` histogram metric that records the duration from workspace agent creation to first connection. This fills an observability gap — provisioner job timings and startup script metrics exist, but the agent connection phase (which can take several minutes) was not exposed to Prometheus. Closes https://github.com/coder/coder/issues/21282 ## Changes - **`coderd/prometheusmetrics/prometheusmetrics.go`** — Define and register a `HistogramVec` in the existing `Agents()` polling loop. Observe `first_connected_at - created_at` exactly once per agent via a deduplication map, pruned each tick to prevent unbounded memory growth. - **`coderd/prometheusmetrics/prometheusmetrics_test.go`** — Update `TestAgents` to set `first_connected_at` on the test agent and assert the histogram is collected with correct labels, sample count, and sample sum. - **`docs/admin/integrations/prometheus.md`**, **`scripts/metricsdocgen/generated_metrics`** — Auto-generated documentation updates from `make gen`. ## Metric details | Property | Value | |---|---| | Name | `coderd_agents_first_connection_seconds` | | Type | histogram | | Labels | `template_name`, `agent_name`, `username`, `workspace_name` | | Buckets | 1s, 10s, 30s, 1m, 2m, 5m, 10m, 30m, 1h | ## Example PromQL ```promql # P95 agent connection time by template histogram_quantile(0.95, sum(rate(coderd_agents_first_connection_seconds_bucket[1h])) by (le, template_name) ) ``` <details> <summary>Implementation notes</summary> ### Design decisions - **Histogram over gauge**: Enables `histogram_quantile()` for percentile queries. - **Observe in `Agents()` polling loop**: All required data is already fetched by `GetWorkspaceAgentsForMetrics()` — no new DB queries. - **Dedup via `map[uuid.UUID]struct{}`**: Prevents re-observing the same agent across polling ticks. Pruned each cycle to bound memory. - **Buckets**: Aligned with `coderd_provisionerd_workspace_build_timings_seconds` range (1s–1h). ### Overhead at scale (100k active workspaces) The deduplication map (`observedFirstConnection`) and per-tick pruning map (`currentAgentIDs`) are both `map[[16]byte]struct{}`. At 100k agents: - **Memory**: ~2.25 MB persistent + ~2.25 MB transient per tick = **~4.5 MB peak**. - **CPU**: ~25 ms of map operations per tick (one tick per minute) = **<0.05% of one core**. Both are negligible relative to the existing cost of the `Agents()` loop (the DB query, per-agent `GetWorkspaceAppsByAgentID` calls, and coordinator node lookups dominate). </details> > 🤖 Generated by Coder Agents |