Skip to content

test(qa): name the failing step in three real-cluster QA flakes - #2856

Merged
freeqaz merged 2 commits into
mainfrom
lows-1018l
Oct 18, 2026
Merged

freeqaz merged 2 commits into
mainfrom
lows-1018l

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 18, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

The 10-18 flake sweep found three new low QA advisory flakes in real-cluster tests. In each case the failure message did not say what had failed:

  • Finding 1884, local installation, "dev-up Keycloak second-pass failure retains recovery state until owned cleanup succeeds" (run 39128476188): dev up failed before the second Helm pass. The assertion's message quoted only the start of stdout, which was cut off with "3927 more characters" left, and never showed stderr.
  • Finding 1883, OpenShell, OAuth2 refresh sources, sandbox-driver-openshell-k3d-real.test.mjs :3430 (run 39131550422, main 23ad336): after the reconfigured source lost its token, the unchanged client-credentials echo call printed something that was not JSON. The test reported only "oauth2-client-credentials must arrive substituted" (undefined !== false).
  • Finding 1882, OpenShell :2706 (run 39130280501): the first Workspace read, right after the Sandbox was ready, answered 503 DEPENDENCY_UNAVAILABLE. The API leaves the cause out of its answer, and the test setup has no API log, so nothing named the failing dependency.

Separately, the console-agents-list-return row in file-durations.json was stale: 17.6 s, against about 25 s since #2837 and #2847 added tests.

Change

Test-only. These are diagnostics only, with no new timeouts or caps.

  • 1884: if dev up's output doesn't match, the predicate throws a new error. It keeps the first line of Node's message, adds dev up's exit code and signal, and appends the last 40 lines of stdout and of stderr, each capped at 6,000 characters. That keeps the whole message under the 16 KiB that the diagnostics report keeps. The original assertion error is attached as cause.
  • 1883: when an echo call's output is not JSON, the test keeps that output, redacted with the file's redactLogLine and turnFailureSecrets and cut to 1,000 characters. The result still has no placeholder or jwt, so every check treats it as a missing token, as before. assertIssued now prints that output. The final check is not polled again. The loop above it polls for the refresh-token loss, not for the client-credentials value, which is read only once after the loop. A transient non-JSON answer, such as a proxy denial or a curl timeout, looks the same as a real failure. So the next sighting should show it, not absorb it. (A first draft re-polled that check; internal review flagged that it could absorb a real failure, so it was dropped.)
  • 1882: on a non-200 first read, the test records what the Workspace read depends on before failing, within the file's diagnostic deadline. The read reaches the Agent Gateway through the Envoy Gateway and the host relay. This change covers the Agent Gateway hop: its Pods (phase, conditions, container readiness and restarts) and the matching lines of their logs. Those lines are redacted, the Gateway API key included, and they keep the Workspace identity's own connect lines, which show whether the read reached the Gateway. The diagnostic then records how a second read answered. The test fails either way. The read is not retried: a 503 right after the Sandbox reports ready may be a real readiness bug, and the next sighting should name it rather than hide it.
  • Durations: console-agents-list-return goes from 17,600 ms to 25,000 ms. That is the median wallDurationMs over the five green main runs ending at 5cc5984 (23.7, 23.7, 25.0, 26.4 and 27.1 s), the method test(ci): refresh three stale file duration rows #2815 and test(console): make the utc-offset log timestamp test fast again #2841 used. The source note says so.

Verification

Risks

Low. The changes are test-only and change nothing on the passing path: each echo output is trimmed once before it is parsed, as before. On the failure path, only 1882 makes a call the test did not make before: one read-only GET, made after the diagnostics are collected, and the test still fails.

Findings 1882, 1883 and 1884 stay open: these changes add diagnostics and don't fix them.

- dev-up Keycloak second-pass case (finding 1884): on a mismatch, the
  failure message carries the last 40 lines of dev up's stdout and stderr
  (each bounded to 6,000 characters) instead of Node's cut stdout quote.
- OpenShell OAuth2 refresh sources (finding 1883): an echo call whose
  output is not JSON keeps that output, redacted and bounded, and the
  substitution assertion prints it. After the reconfigured source loses
  its token, a non-JSON client-credentials echo is logged and polled again
  on the existing loop's 15 s interval and 420 s deadline; a JSON answer
  still ends the poll and is checked as before.
- OpenShell first Workspace read (finding 1882): a non-200 answer records
  the Agent Gateway Pods' state and log and how a second read answered,
  then fails. No retry: a 503 right after the Sandbox is ready may be a
  real readiness bug.
- file-durations.json: console-agents-list-return 17.6 s -> 25.0 s, the
  median of the five green main runs ending at 5cc5984.
@freeqaz
freeqaz requested a review from a team as a code owner October 18, 2026 14:47
@clawsweeper

clawsweeper Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

Review follow-up. The re-poll of a non-JSON client-credentials echo
could absorb a real failure (a proxy denial or a curl timeout looks the
same), and the client-credentials value was only ever read once after
the epoch loop. Drop it: the loop is back to main's, and a non-JSON echo
is printed in the assertion message only.

The Workspace read diagnostic no longer drops the occ-workspace-files
connect lines, which show whether the read reached the Agent Gateway,
and its comment names the other hops (Envoy Gateway, host relay).
@freeqaz
freeqaz temporarily deployed to integration-qa-pr October 18, 2026 14:52 — with GitHub Actions Inactive
@freeqaz
freeqaz temporarily deployed to integration-qa-pr October 18, 2026 14:52 — with GitHub Actions Inactive
@freeqaz-openai

freeqaz-openai commented Oct 18, 2026 •

Copy link
Copy Markdown
Collaborator

Free's clanker here. I found a false pass in the refresh-token check this PR touches. It also reproduces on the parent; this change did not introduce a credential regression.

At 6906357d, if the client-credentials echo is valid but the refresh-token curl exits 28 (timeout, no response) or 52 (empty reply), the test prints “lost the reconfigured token” after that one observation. presented?.valid !== true treats “we didn't get an answer” as “the old sandbox lost access.” The valid echo for a different credential is a useful control, but can't distinguish those outcomes for this request.

I ran the exact response parser, epoch loop, and assertIssued from this head and its parent against synthetic command results. Both accepted those transport failures; both correctly rejected an explicitly valid token minted after reconfiguration, kept waiting on a valid old token, and failed when the control was bad. I then reproduced exits 28 and 52 with real curl against a local test server, alongside a successful control response; the exact PR code printed the loss message on the first observation in both cases. No real credential or cluster was involved.

Could we make the refresh result three-way: confirmed loss, still usable, inconclusive? Record only the curl exit, HTTP status, and a safe recognized echo or refusal class. A timeout must not pass; it can fail immediately as inconclusive. The existing assertCredentialRouteEnforcement in this file already explains why a transport error is not proof of refusal. No change to the final control read, wait budget, or production behavior is needed.

@clawsweeper

clawsweeper Bot commented Oct 18, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper status: review started.

I am starting a fresh review of this pull request: test(qa): name the failing step in three real-cluster QA flakes This is item 1/1 in the current shard. Shard 0/1.

This temporary status tracks the active review worker. The completed review will appear in the durable ClawSweeper review comment.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending. labels Oct 18, 2026
@freeqaz
freeqaz merged commit e787977 into main Oct 18, 2026
53 of 54 checks passed
@freeqaz
freeqaz deleted the lows-1018l branch October 18, 2026 15:03
freeqaz added a commit that referenced this pull request Oct 29, 2026
…he shared helper

Main added a local, unredacted outputTail to dev-up-k3d-real.test.mjs (#2856)
while this branch exports a redacting outputTail from the dev-up helper; the
merge declared the name twice. Use the shared helper so this case's dev up
output is masked like the others.

This branch was previously deployed

1 inactive deployment
integration-qa-pr — 6906357d Deployed Oct 18, 2026 by freeqaz via QA advisory (kubernetes) #2235
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. clawsweeper:intake:needs-repro Preliminary submission classification; full source review pending.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants