Skip to content

Fix malformed HTTP responses from /capabilities and /availability - #27

Open
bjfultn wants to merge 5 commits into
developfrom
fix/vosi-crlf-headers
Open

bjfultn wants to merge 5 commits into
developfrom
fix/vosi-crlf-headers

Conversation

@bjfultn

@bjfultn bjfultn commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Problem

/capabilities and /availability emit their XML document with no HTTP status line and no headers at all — the first bytes on the wire are <?xml version="1.0"....

These are nph- (non-parsed-headers) CGI endpoints, so the script owns the entire response. nginx and Cloudflare reject the result as a bad upstream response, which is what's breaking these two endpoints in the RedHat/NGINX deployment. Both are IVOA requirements.

This is not a CRLF-variant problem. Every other hand-rolled response in the codebase is already correct — tap.py (×11) and vositables.py (×1) all use print("...\r"), which yields a proper \r\n. These two endpoints were simply missing their headers entirely, which is why standard TAP requests were unaffected.

Second defect

Both endpoints fell through to the "retrieve workspace from jobid" branch with an empty jobid. That resolved to <workdir>/TAP and returned a 500 whenever that directory did not already exist.

So on an established server the directory exists, the request reaches the emitter, and you get the header-less response above. On a fresh deployment these endpoints fail outright. They're static documents that need no workspace at all.

Changes

2565a6d — the fix

  • Emit HTTP/1.1 200 OK, Content-type: text/xml, and the blank header-terminating line — all CRLF-terminated, matching the existing convention in this file (print supplies the LF after the literal \r).
  • Dispatch both endpoints before workspace resolution.
  • Add tests/test_vosi_http_response.py.

ca7a755 — deterministic coverage for the workspace defect

The tests above go through the session-scoped tap_server fixture, which shares one TAP_WORKDIR across the suite. By the time they run, earlier sync requests have already created <workdir>/TAP — so a full-suite run only exercised the established-server path. The fresh-deployment case, where the workspace bug actually returned a 500, was covered only by accident when the file happened to run in isolation.

test_vosi_works_without_existing_workspace drives the CGI against a pristine per-test workdir and asserts both a CRLF-terminated 200 and that the endpoints create no workspace at all. Confirmed it pins the second defect specifically: reverting only the dispatch move, keeping the header fix, fails exactly these two tests.

4a12ff9 — unblocking CI (pre-existing, unrelated to the VOSI bug)

ruff check . has been failing on develop since PR #23 merged on 2026-06-02 — a W291 trailing-whitespace hit at TAP/vositables.py:214 introduced by 9024259. The lint step runs before pytest, so the entire test matrix aborted before a single test executed, and the new tests below got no CI signal at all.

requirements-test.txt asked for ruff>=0.1, so CI silently tracked whatever ruff shipped most recently and a newer release began flagging code nobody had touched. Pinned to ruff==0.15.2 so rule changes arrive as a deliberate bump. The vositables.py change is whitespace-only — git diff -w is empty.

Folded in here rather than split out, by request. Happy to break it into its own PR if you'd rather keep this diff to the VOSI files.

Tests

The new tests assert on raw socket bytes rather than going through a forgiving client library — requests would happily paper over exactly the malformation nginx rejects. They check:

  • the response opens with a CRLF-terminated HTTP/1.1 200 OK
  • no bare LF or CR appears within any header line
  • CRLFCRLF terminates the header block
  • an XML Content-type is present
  • the XML body is intact

Verified red/green in both directions: they fail against the unpatched code, and they also catch the HTTP/1.1 200 OK\n bare-LF variant described in the original report, so they'd guard against that regression too.

Full suite: 63 passed, 2 skipped (both skips pre-existing and unrelated). ruff check . clean.

Branching note

develop is a strict superset of main (9 ahead, 0 behind), so this lands on develop. Companion PR #26 carries the same code fix to hotfix/table-validation, which NEA runs in production — that branch is 81 commits behind and predates the HTTP test fixture, so it gets the code fix only.

🤖 Generated with Claude Code

bjfultn and others added 4 commits September 10, 2026 15:34
Both VOSI endpoints emitted their XML document with no HTTP status line
and no headers at all -- the first bytes on the wire were
`<?xml version="1.0"...`. These are nph- (non-parsed-headers) CGI
endpoints, so the script owns the entire response; nginx and Cloudflare
reject the result as a bad upstream response. Every other response in
tap.py already prints a CRLF-terminated status line and headers; these
two were simply missing them.

Emit `HTTP/1.1 200 OK`, `Content-type: text/xml` and the blank
header-terminating line, all CRLF-terminated, matching the existing
convention in this file (print supplies the LF after the literal \r).

Also dispatch these two endpoints before workspace resolution. They are
static documents that need no workspace, but they were falling through
to the "retrieve workspace from jobid" branch with an empty jobid. That
resolved to <workdir>/TAP and returned a 500 whenever that directory did
not already exist -- so on a fresh deployment these endpoints failed
outright, and on an established one they reached the emitter and
produced the header-less response above.

Add tests/test_vosi_http_response.py, which asserts on raw socket bytes
rather than going through a forgiving client library: a CRLF-terminated
status line, no bare LF or CR within any header line, CRLFCRLF ending
the header block, an XML Content-type, and an intact body. Verified
red/green both ways -- the tests fail against the unpatched code and
also catch the `HTTP/1.1 200 OK\n` bare-LF variant.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`ruff check .` has been failing on develop since PR #23 merged
(2026-06-02), on a W291 trailing-whitespace hit at TAP/vositables.py:214
introduced by 9024259. The lint step runs before pytest, so the whole
test matrix aborts before a single test executes.

requirements-test.txt asked for `ruff>=0.1`, so CI silently tracked
whatever ruff had shipped most recently; a newer ruff began flagging
this line on code nobody had touched. Pin ruff==0.15.2 so rule changes
arrive as a deliberate bump rather than a surprise red build.

The vositables.py change is whitespace-only -- `git diff -w` is empty.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The existing VOSI tests go through the session-scoped `tap_server`
fixture, which shares one TAP_WORKDIR across the whole suite. By the
time they run, earlier sync requests have already created
<workdir>/TAP, so a full-suite run only ever exercised the
established-server path -- the fresh-deployment case, where the
workspace-resolution bug actually returned a 500, was covered only by
accident when the file happened to be run in isolation.

Add test_vosi_works_without_existing_workspace, which drives the CGI
directly against a pristine per-test workdir and asserts both that the
response opens with a CRLF-terminated 200 and that the endpoints create
no workspace at all.

Verified it pins the second defect specifically: reverting only the
dispatch move (keeping the header fix) fails these two tests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review preference from @jpl-jengelke on #26, mirrored here to keep the
two branches in sync. Make the CRLF requirement explicit at each print
rather than relying on a trailing \r plus print's implicit \n. Byte
output is unchanged -- both forms emit `...\r\n`, and the wire-level
tests in tests/test_vosi_http_response.py still pass unmodified -- but
the intent is legible without having to remember what print appends,
which matters on a file where a bare LF is what broke the endpoint.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The NEA deployment already answers /capabilities and /availability with
application/xml, and that is the type we want for these documents.  Match
it here so the two do not diverge, and update the wire-level assertion to
match.

Scope is deliberately the two VOSI emitters this PR already touches; the
other Content-type sites (including __printVosiTables__ and the error
paths) are left alone.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.

1 participant