Repository navigation
test(postgres): sign the provisioning administrator in once per file - #2965
Conversation
Every fixture in postgres-agent-provisioning signed the same bootstrap administrator in again (about 210 ms of password hashing each, 64 fixtures, about 13 s of an 87 s file at 2 CPUs). Each fixture's app reads the same stored session (same database and auth secret), so the file now signs in once and reuses that session. No test ends it; tests that need another identity still sign that account in themselves.
…touching the fixture Open #2259 rewrites createFixture's compose/sign-in lines and signs in against a production origin for native-admin cases. Keying the shared administrator session by sign-in options, and wrapping the helper instead of editing createFixture, keeps that PR conflict-free and gives its https origin its own session.
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
P3
…password A test that signs the administrator in with a wrong password to check refusal must not get the cached session. Also reword the wrapper comment: the key is the set of sign-in options.
|
Codex review: needs maintainer review before merge. What this changesThis PR reuses the bootstrap administrator’s authenticated session across PostgreSQL provisioning fixtures while preserving separate sign-ins for other credentials. Example: Run all 55 PostgreSQL provisioning tests against a fresh database.
Review scores
ProductKind: Performance · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep open: this maintainer-authored PR makes a focused fixture optimization, with passing checks on the exact head and no actionable defect found. Priority: P3 Before mergeNone. FindingsNone. Agent review detailsHow this fits togetherThe integration fixtures compose controller applications against shared PostgreSQL storage and send authenticated provisioning requests through Fastify, IAM, and the durable worker. flowchart LR
Fixtures[Provisioning fixtures] --> Cache[File-local session cache]
Cache --> Auth[Controller sign-in]
Auth --> Database[Shared PostgreSQL database]
Cache --> Requests[Fastify requests]
Requests --> IAM[Current IAM authorization]
IAM --> Database
Technical reviewBest possible solution: Retain file-local administrator session reuse while preserving independent fixture applications, live authorization checks, and separate credentials for identity-specific scenarios. Do we have a high-confidence way to reproduce the issue? Current main visibly signs the same administrator credentials in during every fixture creation; the magnitude of the cost comes from author-reported measurements. Is this the best way to solve the issue? The narrow wrapper shares authentication without bypassing request authorization, preserves existing fixture ownership, and avoids overlapping fixture edits in related work. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 5a2cb363d5b2. Provenance checked
TestingProof path: in-process harness. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes: No label changes. Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (1 earlier review cycle)
Reviewed October 19, 2026, 5:16 PM ET / 21:16 UTC (Revision 2). |
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
1 participant
Problem
tests/integration/postgres-agent-provisioning.test.mjsis the slowest file in the PostgreSQL Platform lane, with a CI median of 86.4 s over 15 green main runs. The lane runs 2 files at a time and sums to 263 s, so each second saved in this file cuts about 0.5 s from the lane (median 132 s).Every
createFixturecall signed the same bootstrap administrator in again: 64 sign-ins per run, about 210 ms each of password hashing. Instrumented locally at 2 CPUs, those sign-ins took 13.6 s of an 87.6 s run. Some of that overlaps other async work and some is instrumentation overhead, so the measured saving is about 7 s, not 13.6 s. All other fixture setup together took about 9 s: app composition 1.5 s, Namespace readiness 7.3 s, worker stops 0.4 s.Change
This is test-only. No assertion changes.
createFixture's text is unchanged, but its sign-in call now goes through the file-local wrapper.signInToControllerAppwraps the helper. For the bootstrap administrator it signs in once per set of sign-in options and returns that same session to every later fixture. Every other account, or the administrator with any other password, still signs in on every call, as before (the source-requester and second-administrator cases).Why no state can leak between tests:
revokeCurrentPrincipal) restore them in that fixture's teardown, before the next test starts, exactly as before.Verification
packages/occ/src/index.ts, covering provisioning stop/delete cancellation, the Configuration settle window, retry admission, the request read check, the abandoned-effect wait, replay fingerprints, request authorization and Namespace readiness. Each ran against main's file and this file, each run on a fresh database. The kill sets are equal for every mutant, matched by test name. 9 are killed and 7 survive on both sides.CI estimate: the file drops from 86.4 s to about 79 s (-8%), and the PostgreSQL Platform lane from 132 s to about 128.5 s. The remaining time is mostly waits: about 10 s of retry backoff jitter that the test can't inject, and Namespace readiness.
git merge-treegives no conflict against main or against this head. A first version that editedcreateFixtureconflicted with fix: prepare native admin access on the first Agent deployment #2259, which is why the wrapper exists.Risks