ci: move the release-path actions off the Node 20 runtime - #329
Closed
FrameAutomata wants to merge 7 commits into
Closed
ci: move the release-path actions off the Node 20 runtime#329FrameAutomata wants to merge 7 commits into
FrameAutomata wants to merge 7 commits into
Conversation
FrameAutomata
force-pushed
the
ci/326-node24-third-party
branch
from
August 27, 2026 17:40
65801de to
72d6ec8
Compare
This was referenced Aug 27, 2026
FrameAutomata
force-pushed
the
ci/326-node24-third-party
branch
from
August 27, 2026 17:55
72d6ec8 to
8de17dd
Compare
This was referenced Aug 27, 2026
app/controllers/setup_test.go hardcodes SQLite handles for both db.DB and
db.TelemetryDB, then calls migrations.Run. Under -tags telemetry_duckdb that
dispatches to duckdbTrackingDDL, whose `applied_at TIMESTAMP DEFAULT now()`
is invalid SQLite -- the dialect requires a parenthesised expression for a
function default -- so all six tests in the file fail with:
failed to create schema_migrations table: SQL logic error: near "(": syntax error (1)
Add the missing !telemetry_duckdb clause, matching every sibling file that
hardcodes SQLite handles. dashboard_template_seed_test.go, in this same
package, already carries the three-clause form.
No coverage is lost by excluding rather than adding a tagged DuckDB helper.
transactional_sqlite.go is tagged !transactional_pg, so under
-tags telemetry_duckdb the transactional facade resolves to the same
transactional/sqlite package as the default build; these six tests are
main-DB only and never touch db.TelemetryDB beyond save/restore, so they were
exercising byte-identical code in both builds.
Also widen the DuckDB job's test scope to ./... . Its previous scope skipped
app/controllers, which is why this reached main in the first place. The
comment justifying the narrow scope said three packages fail under this tag;
on c7a5dca exactly one did -- this one -- and with the constraint fixed the
full tree passes.
Verified locally in all three supported combinations:
go test ./... exit 0
CGO_ENABLED=1 go test -tags telemetry_duckdb ./... exit 0
go test -tags "transactional_pg telemetry_ch" ./... exit 0
Closes #309
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* perf: let per-group telemetry reads seek instead of rescanning the window
Every "one group, one time window" query filters (project_id, <group>,
recorded_at), but no index covered the time column. SQLite therefore seeks
the (project_id, recorded_at) index and re-filters the group column across
the whole window -- once per group, for every group in range, since the
grouped-list loop paginates in Go only after computing percentiles.
Widen the two group indexes and add the missing tasks one. endpoints and
ai_traces are widenings: (project_id, <group>) stays a prefix, so every
query that used the old index is still covered, and no query on these
tables filters the group column without a time bound.
Measured on a 730MB / 2.5M-row telemetry DB seeded over a 30-day window,
without ANALYZE (no deployment runs it):
24h window 7d window
endpoints 6.38s -> 159ms 46.9s -> 1.29s
tasks 81ms -> 13ms 736ms -> 117ms
ai traces 108ms -> 13ms 1.03s -> 111ms
Write cost: the widenings are single-digit percent. The one genuinely new
index roughly halves tasks insert throughput (~120k -> ~64k rows/s), which
is a real relative cost but leaves ample headroom for background-task
volume. Building the indexes adds ~3s to the first boot after upgrade, and
migrations complete well before the listener binds, so it is slower start
rather than downtime.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test: run the real migrations instead of a hand-copied schema
The telemetry test helper carried its own 230-line copy of the schema plus
a private re-implementation of migrations.splitStatements. Landing the
index change above meant hand-editing that copy to match -- which is the
tell that the copy is the problem.
It had already drifted, in the same way, more than once:
- metric_points still declared idx_metric_points_project_name, which
migration 0006 DROPPED, and never gained the widened replacement --
the identical drop-and-widen pattern as 0020
- session_recordings declared an invented idx_session_recordings_session_id
on (session_id); the real index is idx_session_recordings_session on
(session_id, segment_index)
- check_results was absent entirely, though its repository exists
- idx_exceptions_session, idx_log_records_project_trace,
idx_log_records_project_service and idx_ai_traces_project_trace_name
were all missing
dbtest.SetupSQLite already does this properly and is used by five other
test packages. Route the helper through it: the copy and the duplicated
splitStatements go away, the suite exercises the real 20-migration schema,
and the 105 setupTestDB call sites are untouched. Package runtime goes
0.30s -> 2.19s, the cost of applying the real migration set per setup.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
`GetTraceNameStats` on the SQLite telemetry backend swallowed the error from its percentile query with `return stats, nil`. On any failure -- context deadline on a large trace_name, a locked telemetry DB, a scan type error -- the caller got a struct with a real Count, AvgDuration and token totals next to MedianDuration and P95Duration of exactly zero, and nothing was logged or captured. Zero is a plausible latency, so this reads as a fast trace rather than a failed read. The DuckDB and ClickHouse backends compute the percentiles inside the aggregate query and propagate the error, and the SQLite endpoint and task equivalents already `return nil, err`, so this was the one path that fabricated a value. The route treats stats as best-effort -- it nils them and still answers 200 with the trace list -- so propagating the error does not turn this into a 500. It swaps a misleading zero for an absent stats block, and the discard in the controller now reports through CaptureException per the non-stopping-error convention rather than vanishing. No regression test: both queries in this function read the same table, so there is no way to fail only the percentile read from a test without injecting a fake executor, which the repositories do not support. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
`sortEndpointStats` produced its descending order by negating the ascending comparison. For two rows that tie on the sort key that returns true for both (i, j) and (j, i), which is not the strict weak ordering sort.Slice documents a requirement for, so the result is formally unspecified. Descending is now the same comparison with the operands swapped. This is hygiene, not a bug fix. I could not make the old comparator misbehave: 2400 trials over slices of 8 to 20000 elements, 1 to 100 distinct values and three input patterns produced zero misordered results and zero lost elements, so Go's pdqsort tolerates the `>=` comparator in practice today. What the change buys is not depending on that tolerance. No test, because none can distinguish the two versions -- any behaviour a test could asserts holds for the old comparator too, which is what the probe above measured. The sibling sort in task.repository.go already used the strict form; this brings the two endpoint repositories in line with it. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The six actions #326 left after the actions/* sweep, all third-party and all on release workflows: docker/build-push-action v5 -> v7 (release-traceway, 5 uses) docker/setup-qemu-action v3 -> v4 (release-traceway) docker/setup-buildx-action v3 -> v4 (release-traceway) docker/login-action v3 -> v4 (release-traceway) cloudflare/wrangler-action v3 -> v4 (release-website, -docs, -helm) azure/setup-helm v4 -> v5 (release-helm) Every pin in .github/workflows is node24 or composite after this. No PR label exercises any of these, so this wants a workflow_dispatch dry run before merging -- see the PR description for which and in what order. wrangler-action v4 changed its default Wrangler CLI from 3 to 4. Neither website/package.json nor docs/package.json depends on wrangler, so the action's default is what actually deploys tracewayapp.com and the docs: taking v4 unpinned would carry a CLI major bump into a production deploy inside a runtime change. `wranglerVersion: "3"` holds the CLI where it is so this change moves one thing. Wrangler 4 is worth doing, deliberately and on its own. The other five are inert for how they are called here: build-push-action v6 turned on build summaries (default on, additive) and v7 dropped the DOCKER_BUILD_NO_SUMMARY and DOCKER_BUILD_EXPORT_RETENTION_DAYS envs -- neither is set anywhere in the repo. The inputs in use (context, file, push, tags, platforms, cache-from, cache-to) are unchanged. setup-buildx-action v4 removed deprecated inputs and outputs. It is invoked with no inputs and its outputs are not referenced -- the step has no id. login-action v4, setup-qemu-action v4 and setup-helm v5 are Node 24 plus ESM only. login-action's registry/username/password are unchanged, and the other two take no inputs. Refs #326. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…in an owner build-push-action v6+ generates a build summary and uploads a build record artifact per build. release-traceway.yml runs five of them, so the bump silently added five artifacts per release inheriting the repo-wide retention. The summary itself earns its keep -- all five builds write cache-to: type=gha,mode=max and it is the only place per-build cache-hit ratios are visible without re-running a release -- so this caps the record retention at a week rather than disabling the feature. The three wranglerVersion comments deferred to release-website.yml for their rationale, which is a coupling nothing enforces: each of these files is normally read alone in a diff, and rewording or retiring the website comment would silently orphan two pointers. Each site now states its own reason. The pin's exit condition was also prose and nothing else, on a production deploy path, in a repo where unowned version state drifting until it becomes an annotation is the whole subject of #326. Filed as #331 and referenced from all three sites. #330 covers the absent drift detection that let #326 happen twice in one day. Refs #326. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FrameAutomata
force-pushed
the
ci/326-node24-third-party
branch
from
August 28, 2026 22:26
8de17dd to
245cc4f
Compare
Collaborator
Author
|
Reopening against Also corrected the validation section, which told reviewers to dry-run these with |
FrameAutomata
added a commit
that referenced
this pull request
Aug 28, 2026
Nothing in this repo watched action versions: no dependabot.yml, no renovate.json, and the strings appear nowhere. That is how the Node 20 runtime migration (#326) was found twice in one day, both times because a human read a run annotation. #328/#329 bought distance from that deadline but no detection, so the next major lands exactly as silently. Adopt Dependabot for the github-actions ecosystem only. npm and Go modules are left unwatched on purpose -- backend/ and cli/ already run govulncheck daily, and those ecosystems would bury the signal this file exists for. Two groups, split the same way #328/#329 split by hand, on what a major means rather than on who ships it: actions/* majors are normally runtime bumps, wide and mechanical and reviewable as one sweep, while a third-party major can change behaviour -- wrangler-action v4 silently moved its default Wrangler CLI from 3 to 4, which is the whole of #331. The PRs are deliberately not auto-labelled `ci`. That label is the collaborator trust gate that makes PR CI opt-in, and an unattended bot PR proposing an action version nobody has read should not trip it; a reviewer applies it after looking, as for any human PR. Worth being clear-eyed that this is mostly detection and not validation: `ci` reaches only backend*.yml and cli*.yml, while most pins sit on release-*.yml, benchmark-*.yml and traceway-autofix.yml, where validating a bump still means a workflow_dispatch dry run. Detection is the half that was missing. Settle the two Node questions in the same pass, since they are the same discipline. release-docs.yml and release-website.yml pinned `node-version: "latest"`, which floats two production deploys onto whatever Node shipped that morning, up to and including a non-LTS Current release -- and neither docs/package.json nor website/package.json declares an engines floor to catch it. Both now pin 22, the major already used by release-traceway.yml, benchmark-processor.yml and the node:22-alpine Dockerfiles, matching frontend/package.json's >=22. Whether Traceway moves to Node 24 is now one decision made once rather than one made silently on two deploy paths. The source-map upload recipe was the last surviving `node-version: 20` in the repo, recommending to readers a major this repo uses nowhere while #328 had just bumped the action pins around it to the Node 24 majors. It recommends 22 now, the same version the shipped Dockerfiles run. Closes #330 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 2 of #326, stacked on #328 (which does the 63
actions/*pins). Merge #328 first; this one retargets tomainautomatically.These are the six actions #328 left — all third-party, all on release workflows. After this, every pin in
.github/workflowsisnode24or composite.docker/build-push-actionrelease-traceway(5 uses)docker/setup-qemu-actionrelease-tracewaydocker/setup-buildx-actionrelease-tracewaydocker/login-actionrelease-tracewaycloudflare/wrangler-actionrelease-website,-docs,-helmazure/setup-helmrelease-helmEvery workflow touched here is
workflow_dispatch/release-triggered, so thecilabel validates none of it. Suggested order, cheapest first:release-helm.yml— dispatch. Exercisessetup-helm@v5and awrangler-action@v4Pages deploy, and publishes only a chart index.release-docs.ymlorrelease-website.yml— dispatch. The secondwrangler-actionshape (command: deploy, Workers rather than Pages).release-traceway.yml— the fourdocker/*bumps together. Biggest blast radius; worth doing last and watching the multi-archsqlitebuild specifically.wrangler-actionv4 changes the Wrangler CLI major — pinned herev4's headline change is that its default Wrangler CLI moves from 3 to 4. That matters more than it looks: neither
website/package.jsonnordocs/package.jsondepends onwrangler, so there is no project-pinned CLI for the action to pick up — its default is literally what deploystracewayapp.comand the docs.Taking v4 unpinned would smuggle a CLI major bump into a change whose entire purpose is a runtime bump, on a production deploy path, in a workflow no PR can test. So all three call sites now pin:
Same CLI as today, new runtime. Wrangler 4 is worth doing — as its own change, with its own dry run. Dropping the pin is all it takes.
Why the other five are inert here
build-push-actionv6 turned on build summaries (default on, additive — a summary panel plus an exported build record per build). v7 removed the deprecatedDOCKER_BUILD_NO_SUMMARYandDOCKER_BUILD_EXPORT_RETENTION_DAYSenvs; neither appears anywhere in the repo. Every input in use —context,file,push,tags,platforms,cache-from,cache-to— is unchanged.setup-buildx-actionv4 removed deprecated inputs and outputs (#464). It's invoked with no inputs, and its outputs can't be referenced — the step has noid.login-actionv4,setup-qemu-actionv4 andsetup-helmv5 are Node 24 + ESM only.login-action'sregistry/username/passwordare unchanged; the other two take no inputs at all.All runners are GitHub-hosted (
ubuntu-latest/ubuntu-24.04), so the ≥ 2.327.1 runner requirement is already met.🤖 Generated with Claude Code
From a cleanup pass over this diff
The build summary is not free, and the bump turned it on silently.
build-push-actionv6+ generates a summary and uploads a build-record artifact per build.release-traceway.ymlruns five builds, so this bump quietly added five artifacts per release, inheriting the repo-wide retention.Kept rather than disabled — all five builds write
cache-to: type=gha,mode=max, and the summary panel is the only place per-build cache-hit ratios and step timings show up without re-running a 30-minute release. Bounded instead withDOCKER_BUILD_RECORD_RETENTION_DAYS: 7at the job level: a week is long enough to debug the release that produced them. (Note the env var is the v7 spelling — v7 removed the olderDOCKER_BUILD_NO_SUMMARYandDOCKER_BUILD_EXPORT_RETENTION_DAYSnames.)The
wranglerVersioncomments no longer defer across files. Two of the three said "See release-website.yml" for their rationale, which is a coupling nothing enforces — each of these files is normally read alone in a diff, and rewording or retiring the website comment would silently orphan two pointers. Each site now states its own reason.The pin now has an owner. Its exit condition was prose in a comment and nothing else, on a production deploy path, in a repo where unowned version state drifting until it becomes an annotation is the entire subject of #326. Filed as #331 and referenced from all three sites, so the deferral survives #326 being closed.