Skip to content

Fix malformed HTTP responses from /capabilities and /availability (NEA hotfix branch) - #26

Merged
bjfultn merged 3 commits into
hotfix/table-validationfrom
fix/vosi-crlf-headers-nea
Sep 11, 2026
Merged

bjfultn merged 3 commits into
hotfix/table-validationfrom
fix/vosi-crlf-headers-nea

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

  • 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.

Verification

Driven directly against the CGI for both endpoints:

HTTP/1.1·200·OK␍␊
Content-type:·text/xml␍␊
␍␊
<?xml·version="1.0"·encoding="UTF-8"?>␊

Before the patch the same command returned <?xml... as the very first line. Confirmed working both with and without <workdir>/TAP present. Existing suite: 10 passed.

Note on scope

This branch is 81 commits behind main and predates the HTTP test fixture, so it carries the code fix only. The companion PR against develop includes wire-level regression tests.

🤖 Generated with Claude Code

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.

Verified by driving the CGI directly for both endpoints: the response
now begins with a CRLF-terminated 200 status line, and works whether or
not <workdir>/TAP exists.

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

@jpl-jengelke jpl-jengelke left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The only thing I prefer is a different format for printing the header lines.

    print('HTTP/1.1 200 OK', end='\r\n')
    print('Content-Type: text/plain', end='\r\n')
    print('', end='\r\n')

Why? It flushes immediately and makes clear the intent. We had a lot of problems with CRLF requirements as proxies are very particular.

@tobular

tobular commented Sep 10, 2026

Copy link
Copy Markdown

Fantastic! Spot on, and I understand nea is behind main so not an issue, I can't merge but I'll quickly pull the revision branch and provide feedback shortly

Review preference from @jpl-jengelke: 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` -- 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>
bjfultn added a commit that referenced this pull request Sep 10, 2026
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>
@bjfultn

bjfultn commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

Adopted on both branches — fix/vosi-crlf-headers-nea (this PR) and fix/vosi-crlf-headers (#27), so the two stay in sync:

print ('HTTP/1.1 200 OK', end='\r\n')
print ('Content-type: text/xml', end='\r\n')
print ('', end='\r\n')

The readability argument is the one that sold it. Both forms emit byte-identical ...\r\n, but the explicit terminator puts the requirement at the call site instead of relying on the reader remembering what print appends — the right default on a file where a bare LF is precisely what broke the endpoint.

One correction on the rationale, since it could bite elsewhere: end='\r\n' does not flush. end= only sets the terminator string; flushing needs flush=True or an unbuffered/line-buffered stream.

$ python -c "print('A', end='\r\n'); import os; os.write(1, b'B')"
BA

The raw os.write to fd 1 lands before the buffered print — A\r\n was still sitting in the buffer.

It happens not to matter for these two endpoints: both print everything and then sys.exit(), and Python flushes stdout at exit. Flagging it in case the team is counting on end= to flush somewhere it does matter, e.g. interleaving buffered print with sys.stdout.write or os.write on the same fd — there you'd want flush=True.

Kept Content-type rather than Content-Type to match the other 15 occurrences in tap.py (all lowercase-t; field names are case-insensitive per RFC 9110 §5.1). Happy to standardize on the canonical casing if you'd prefer, but that felt like a separate sweep rather than something to change in two places here.

@bjfultn

bjfultn commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator Author

Live test-server results: /availability fixed, /capabilities still broken — but not by this branch

Followed up on @tobular's report that the fix took for /availability but not /capabilities. Reproduced against https://exoplanetarchive.nexsci.services and traced it. Short version: the remaining failure is a fall-through in the deployed build, and the deployed build is not the code on this branch.

What the live server returns

Endpoint Result
/availability 407 bytes, well-formed XML. Fixed.
/tables 1,565,567 bytes, well-formed XML. Fine.
/capabilities HTTP 200 with correct CRLF headers and a valid capabilities document — followed by a second complete HTTP response embedded in the first one's body.

The seam in the /capabilities body:

90  </vosi:capabilities>
91  \r
92  HTTP/1.1 400 ERROR\r
93  Content-type: application/xml\r
94  \r
95  <?xml version="1.0" encoding="UTF-8"?>
96  <VOTABLE version="1.4" xmlns="http://www.ivoa.net/xml/VOTable/v1.3">
97  <RESOURCE type="results">
98  <INFO name="QUERY_STATUS" value="ERROR">
99  Input 'query' is blank.

That trailing block is what produces the browser's "Extra content at the end of the document". The HTTP framing is fine; the body is polluted.

Root cause

__printVosiCapability__ emits its XML and then returns instead of terminating. Execution falls through into the normal query-handling path, which finds no QUERY parameter and emits its own complete 400 response into the already-open 200.

Confirmed by taking 043192b from this branch, replacing the sys.exit() at the end of __printVosiCapability__ with pass, and running it against a local fixture:

simulated (this branch minus sys.exit)   live server
93  </vosi:capabilities>                 90  </vosi:capabilities>
                                         91  \r
94  HTTP/1.1 400 ERROR\r                 92  HTTP/1.1 400 ERROR\r
95  Content-type: text/xml\r             93  Content-type: application/xml\r
96  \r                                   94  \r
97  <?xml version="1.0" ...              95  <?xml version="1.0" ...

Same failure, byte for byte.

The deployed build is not this branch

Two tells in the live bytes:

  1. Live emits Content-type: application/xml. This branch emits text/xml, in both VOSI emitters and in __printError__.
  2. Live has a stray print("\r") after </vosi:capabilities> that this branch does not have.

So NEA is running a different patch. That one fixed the headers on both endpoints and fixed availability's exit, but dropped the sys.exit() at the end of the capabilities emitter.

This branch does not have the bug. Running 043192b directly against a pristine TAP_WORKDIR:

/availability   405 bytes   HTTP/1.1 200 OK\r\n   well-formed XML
/capabilities  3547 bytes   HTTP/1.1 200 OK\r\n   well-formed XML

Both terminate cleanly, neither creates a workspace. Same result on the develop-based branch in #27, which is covered by the regression tests there.

Fix is either one line — restore sys.exit() at the end of __printVosiCapability__ in the deployed variant — or deploy this branch, which already has it.

Cloudflare is not involved: cf-cache-status: DYNAMIC on both endpoints, so nothing stale is being served.

Unrelated issue, but worth flagging

The public capabilities document advertises internal URLs:

<accessURL use="full">http://vmexoweb2.ipac.caltech.edu:8080/TAP</accessURL>
<accessURL use="full">http://vmexoweb2.ipac.caltech.edu:8080/TAP/tables</accessURL>
<accessURL use="full">http://vmexoweb2.ipac.caltech.edu:8080/TAP/capabilities</accessURL>
<accessURL use="full">http://vmexoweb2.ipac.caltech.edu:8080/TAP/availability</accessURL>

Any IVOA client that reads /capabilities for endpoint discovery (TOPCAT, pyvo) will be sent to that internal host and port rather than https://exoplanetarchive.nexsci.services. That is HTTP_URL / HTTP_PORT in the deployment's TAP.conf, not a code issue — but it will start mattering the moment /capabilities parses again.

@bjfultn

bjfultn commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator Author

Correction: this branch is fine — the live server is running a stale build

My previous comment concluded that the capabilities emitter had lost its sys.exit(). That is wrong about this branch, and I want to correct it before it misleads anyone reviewing.

fb52105 ("additional http formatting") landed on this branch after I pulled, and I had not seen it when I wrote that. It keeps sys.exit(). I ran it against the local fixture:

/availability   rc=0  http-responses-in-output=1  well-formed XML
/capabilities   rc=0  http-responses-in-output=1  well-formed XML
head: HTTP/1.1 200 OK\r\nContent-type: application/xml\r\nConnection: close\r\n\r\n

One response per request, no fall-through, no trailing 400. The branch as it stands is correct.

What fb52105 also explains: the stray \r I flagged between </vosi:capabilities> and the HTTP/1.1 400 ERROR block comes from the print (end='\r\n') it adds after each VOSI document. That is a real line in this branch, not evidence of a mangled patch.

So the diagnosis stands but the conclusion flips. The fall-through on https://exoplanetarchive.nexsci.services/cgi-bin/TAP/nph-tap.py/capabilities is real and still reproducible as of this writing, but it comes from an intermediate build deployed between 295d5f1 and fb52105 — after the trailing-CRLF line was added, before the exit was restored. There is no code change left to make here: /capabilities needs a redeploy, not a patch.

Worth confirming after redeploy:

curl -sS https://exoplanetarchive.nexsci.services/cgi-bin/TAP/nph-tap.py/capabilities \
  | grep -c 'HTTP/1.1'

Should print 0. Today it prints 1, because a second complete HTTP response is embedded in the body.

Non-blocking follow-up

fb52105 switches Content-type to application/xml at nine sites. For the VOSI documents that is right and matches what we want. Two of the other sites emit VOTable, though, where the registered IVOA type is application/x-votable+xml — the type this service's own capabilities document advertises under <mime>:

<outputFormat ivo-id="ivo://ivoa.net/std/TAPRegExt#output-votable-td">
  <mime>application/x-votable+xml</mime>
  <alias>votable</alias>
</outputFormat>

application/xml matches neither the old text/xml nor the advertised type, and pyvo and TOPCAT do inspect Content-type. Not a reason to hold this PR — the VOSI fix is what is urgent — but worth a follow-up to decide whether those two sites should be application/x-votable+xml instead.

Also unchanged from my last comment

The public capabilities document still advertises http://vmexoweb2.ipac.caltech.edu:8080/TAP... in its accessURL elements. That is HTTP_URL / HTTP_PORT in the deployment's TAP.conf, not a code issue, but any client doing endpoint discovery will follow those internal URLs.

@bjfultn
bjfultn merged commit bbc1108 into hotfix/table-validation Sep 11, 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.

3 participants