Skip to content

itest: boot piri on Postgres so the stack comes up; accept any 2xx CORS preflight - #47

Closed
bajtos wants to merge 2 commits into
mainfrom
claude/cors-test-failures-2o3ii9
Closed

itest: boot piri on Postgres so the stack comes up; accept any 2xx CORS preflight#47
bajtos wants to merge 2 commits into
mainfrom
claude/cors-test-failures-2o3ii9

Conversation

@bajtos

@bajtos bajtos commented Jul 28, 2026

Copy link
Copy Markdown
Member

Fixes #45. The itest suite is green again — see the note below, because the root cause is not the one #45 diagnosed.

The failure was stack boot, not the CORS test

Every top-level itest failed, and none of them reached an assertion:

Error: initializing piri node: ... ProvideHarmonyDB ...
curio PDP pipeline requires Postgres (set database type to postgres)

Since piri:main absorbed Curio (~2026-07-24) its PDP pipeline refuses sqlite, and forgeStack booted sqlite piri via the zero-value PiriNodeConfig{}. This is pre-existing on main: main's own last itest run (c57186b4, the #42 merge this branch is based on) fails identically — same four suites, 24 occurrences of that error.

All four suites route through that one constructor, so the fix is one line: stack.WithPiriNodes(stack.PiriNodeConfig{Postgres: true}). The pinned smelt already supports it (PiriNodeConfig.Postgresmanifest.DBPostgres), so no dependency bump is involved.

Correction to the #45 diagnosis: the versitygw pin is fine

#45 concluded that the pinned versitygw v0.0.0-20260716095011 predates the CORS work and that the fix required bumping to a fork build with preflight support. That isn't the case — that exact module version already ships the whole stack:

  • OPTIONS /:bucket and /:bucket/* routes (s3api/router.go:1547-1561)
  • the CORSOptions controller (s3api/controllers/options.go)
  • ApplyBucketCORS on every bucket/object route, and ApplyBucketCORSPreflightFallback

Reproducing the itest's exact requests against an in-process server (real versitygw s3api + ingot's config→CORS-document path) shows every header assertion already passing on the current pin: Allow-Origin echoed on the https://*.dev.example wildcard match, Allow-Methods including PUT, Max-Age 600, a bare 403 for a disallowed origin, and Expose-Headers including ETag on a presigned cross-origin GET. No versitygw bump is needed.

The one genuine test defect was the status: versitygw's ProcessController renders every successful controller response as 200 unless MetaOptions.Status is set, and CORSOptions doesn't set it. 204 is only what the no-CORS-config fallback returns — a path ingot never takes, since it reports a CORS configuration for every bucket.

So the assertion now accepts any 2xx, the fetch spec's "ok status" range. Nothing is lost in strictness: the Max-Age == "600" check is what actually pins the real CORS path, because the fallback sets no Max-Age and mirrors the requested method instead of the rule's method list.

Overlap with #44

#44 independently arrived at both of these same changes (piri-on-Postgres, and the 2xx relaxation) as incidental fixes alongside its catalog-retention work. The overlap resolves trivially whichever lands first. #44's note that TestForgeReadAfterEviction would still fail on an upstream piri did:plc proof-verification problem did not reproduce here — it passed in 127.66s.

Verification

itest job green on 574a0a5 (run 30366470280), all four suites passing and TestForgeScenarios/CORS genuinely running rather than skipped:

--- PASS: TestForgeReadAfterEviction (127.66s)
--- PASS: TestForgeNativeProvision (48.71s)
--- PASS: TestForgeScenarios (50.76s)
    --- PASS: TestForgeScenarios/CORS (0.08s)
--- PASS: TestForgeVersity (92.62s)

Zero failures; the remaining skips are the pre-existing curated XFail rows and the documented position-dependent cases. Unit suite green on all three platforms, gofmt clean, go vet -tags itest ./itest clean.

TestForgeScenarios/CORS (added in #42/#43) has never passed: it asserted
a matched preflight is answered 204, but versitygw renders every
successful controller response — CORSOptions included — as 200 unless
MetaOptions.Status says otherwise, which matches real S3 (browsers
accept any 2xx for a preflight). 204 is only the status of versitygw's
no-CORS-config preflight fallback, a path ingot never hits once
cors_allowed_origins reports a configuration for every bucket.

Contrary to the diagnosis in #45, the pinned versitygw
(v0.0.0-20260716095011) already ships the preflight route and CORS
middlewares; reproducing the itest's requests against an in-process
server shows every header assertion (Allow-Origin echo on a wildcard
match, Allow-Methods incl. PUT, Max-Age 600, disallowed origin bare,
presigned-GET Expose-Headers incl. ETag) passing — the status was the
sole failure, exactly the one line CI reported.

Fixes #45.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017rwJ8KiPhm3G27vfU21Phk

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the Forge integration scenario test to expect a 200 OK response for a successful CORS preflight, aligning the test with observed S3-style behavior and typical browser expectations for OPTIONS preflight handling.

Changes:

  • Update the TestForgeScenarios/CORS preflight status assertion from 204 No Content to 200 OK.
  • Add an explanatory comment clarifying why 200 is expected for a matched preflight.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Every top-level itest — TestForgeVersity, TestForgeScenarios,
TestForgeNativeProvision, TestForgeReadAfterEviction — failed at stack
boot, not in any assertion:

  Error: initializing piri node: ... ProvideHarmonyDB ...
  curio PDP pipeline requires Postgres (set database type to postgres)

Since piri:main absorbed Curio (~2026-07-24) its PDP pipeline refuses
sqlite, and forgeStack booted sqlite piri via the zero-value
PiriNodeConfig. All four suites route through that one constructor, so
flipping the topology default to Postgres:true is the whole fix; the
pinned smelt already supports it (no dependency bump).

Also relax the CORS preflight status assertion from 200 to any 2xx, the
fetch spec's 'ok status' range. versitygw answers a matched rule 200 and
only its no-CORS-config fallback 204, and the Max-Age assertion is what
actually pins the real path: the fallback sets no Max-Age and mirrors the
requested method instead of the rule's method list. This supersedes the
exact-200 assertion in 900d6b5.

Both changes match what #44 arrived at independently, so the overlap
resolves trivially whichever lands first.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017rwJ8KiPhm3G27vfU21Phk
@bajtos bajtos changed the title Fix CORS preflight response to return 200 instead of 204 itest: boot piri on Postgres so the stack comes up; accept any 2xx CORS preflight Jul 28, 2026
@bajtos
bajtos marked this pull request as ready for review July 28, 2026 14:14
@bajtos
bajtos requested review from alanshaw and frrist July 28, 2026 14:15
Comment thread itest/stack_test.go
stack.WithPiriNodes(stack.PiriNodeConfig{}),
// Postgres-backed piri: piri:main's curio PDP pipeline refuses
// sqlite ("curio PDP pipeline requires Postgres") as of 2026-07-24.
stack.WithPiriNodes(stack.PiriNodeConfig{Postgres: true}),

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to change stack.WithPiriNodes to configure Postgres by default. Perhaps as part of the larger cleanup to remove SQLite from Piri & Smelt.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that's the right end state — but WithPiriNodes / PiriNodeConfig live in fil-forge/smelt, so flipping the default can't land in this PR. Keeping this one line here is the minimum to get a red suite green against the smelt version ingot currently pins.

Worth noting for whenever the smelt cleanup happens: this line can't rot silently either way. If smelt just flips the default, Postgres: true becomes redundant but stays correct. If the field goes away entirely along with sqlite, this becomes a compile error at bump time — so it surfaces immediately rather than quietly reverting the suite to sqlite.

Happy to pick up the smelt-side change (invert or drop the field, then drop this argument here) as a follow-up if you want to point me at it — it's outside this repo, so it needs its own PR there.


Generated by Claude Code

bajtos referenced this pull request Jul 28, 2026
The subtest (added in #42/#43) asserts preflight behavior the pinned
versitygw does not implement, and merged while the itest suite could
not boot a stack — so it has never passed in any environment. Skip
with a pointer to #45 rather than leave the suite red for a failure
that belongs to the CORS feature, not whichever branch runs it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@alanshaw alanshaw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was resolved in #46

@bajtos bajtos closed this Jul 28, 2026
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.

TestForgeScenarios/CORS has never passed: preflight support missing from the pinned versitygw

4 participants