Add faithful private Tinker checkpoint serving - #464
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| active_requests += 1 | ||
| in_flight = active_requests | ||
| log_event("start", request_id=request_id, in_flight=in_flight) | ||
| body = json.loads(self.rfile.read(int(self.headers["content-length"]))) |
There was a problem hiding this comment.
Request changes: malformed requests bypass the OpenAI error mapping here. json.loads(...) and the Content-Length lookup occur before the protected try, so invalid JSON or a missing/invalid length causes BaseHTTPRequestHandler to terminate the request with a traceback/connection failure instead of returning a bounded 4xx JSON envelope. Please move body reading/parsing into the guarded path and add a provider-free regression for malformed JSON (and ideally non-object JSON / missing messages) to preserve the claimed fail-closed compatibility contract.
| with active_lock: | ||
| active_requests += 1 | ||
| in_flight = active_requests | ||
| log_event("start", request_id=request_id, in_flight=in_flight) | ||
| body = json.loads(self.rfile.read(int(self.headers["content-length"]))) | ||
| try: |
There was a problem hiding this comment.
🔴 Malformed requests leave the server permanently counting phantom work and get no reply
The incoming request body is read and decoded (json.loads(self.rfile.read(...)) at scripts/tinker-openai-shim.py:201) outside the protected block that decrements the in-flight counter, so a request with a missing or non-numeric length header or invalid JSON aborts with no reply and leaves the counter permanently inflated.
Impact: Bad or truncated client requests get a dropped connection instead of an error response, and the reported in-flight load drifts upward forever, making operational logs useless.
Counter increment happens before the try/finally that decrements it
scripts/tinker-openai-shim.py:197-201 increments active_requests and logs start, then reads the body. The try:/finally: that decrements active_requests only begins at line 202. Any exception from self.headers["content-length"] (KeyError), int(...) (ValueError), or json.loads (JSONDecodeError) propagates out of do_POST, so:
- no response is written (the client sees a closed connection rather than an OpenAI-shaped 400),
active_requestsis never decremented, corrupting every subsequentin_flightlog value.
| with active_lock: | |
| active_requests += 1 | |
| in_flight = active_requests | |
| log_event("start", request_id=request_id, in_flight=in_flight) | |
| body = json.loads(self.rfile.read(int(self.headers["content-length"]))) | |
| try: | |
| with active_lock: | |
| active_requests += 1 | |
| in_flight = active_requests | |
| log_event("start", request_id=request_id, in_flight=in_flight) | |
| try: | |
| body = json.loads(self.rfile.read(int(self.headers["content-length"]))) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| message = normalize_assistant_message(message, request_id) | ||
| payload = build_chat_completion( | ||
| message, | ||
| prompt_tokens, | ||
| completion_tokens, | ||
| finish_reason, | ||
| model=requested_model, | ||
| request_id=request_id, | ||
| ) |
There was a problem hiding this comment.
🟡 Tool-call replies are labelled as ordinary completions
A reply that asks to run a tool is still reported as a plain finished answer (normalize_finish_reason at scripts/tinker-openai-shim.py:140-144 only ever yields "stop" or "length"), so callers that decide what to do next from that label will treat a tool request as final text.
Impact: Agent runners that branch on the completion label can stop the conversation instead of executing the requested tool, breaking multi-turn tool use.
OpenAI contract requires finish_reason "tool_calls"
The OpenAI chat-completions contract sets finish_reason: "tool_calls" whenever message.tool_calls is non-empty. Here sample() computes the finish reason purely from the sampler stop reason / renderer termination / token cap, and normalize_finish_reason in scripts/tinker_openai_compat.py:120-151 can only return "stop" or "length". Since the shim explicitly supports tool calls (normalize_assistant_message at scripts/tinker_openai_compat.py:76-105), the emitted envelope is inconsistent with the documented "strict OpenAI chat-completion response envelope" goal.
A fix is to override the finish reason to "tool_calls" after normalize_assistant_message when the normalized message contains tool calls.
| message = normalize_assistant_message(message, request_id) | |
| payload = build_chat_completion( | |
| message, | |
| prompt_tokens, | |
| completion_tokens, | |
| finish_reason, | |
| model=requested_model, | |
| request_id=request_id, | |
| ) | |
| message = normalize_assistant_message(message, request_id) | |
| if message.get("tool_calls"): | |
| finish_reason = "tool_calls" | |
| payload = build_chat_completion( | |
| message, | |
| prompt_tokens, | |
| completion_tokens, | |
| finish_reason, | |
| model=requested_model, | |
| request_id=request_id, | |
| ) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| if wildcard: | ||
| receipt.update( | ||
| compatibility="incompatible", | ||
| reason=( | ||
| "wildcard all-linear training includes Nemotron-H Mamba and routed-MoE " | ||
| "targets that vLLM cannot faithfully represent" | ||
| ), | ||
| unsupported_target_modules=[wildcard], | ||
| ) | ||
| return receipt | ||
| if not targets: | ||
| receipt.update( | ||
| compatibility="incompatible", | ||
| reason="adapter target_modules is missing or invalid; compatibility must fail closed", | ||
| unsupported_target_modules=["missing_or_invalid"], | ||
| ) | ||
| return receipt |
There was a problem hiding this comment.
🟡 Compatibility receipt blames a wildcard setting when the adapter config is actually missing or malformed
A configuration whose module list is absent or of the wrong type is described in the receipt as wildcard all-linear training (if wildcard: at scripts/adapter-serving-compat.py:61) instead of as missing/invalid, so the emitted reason misstates why the check failed.
Impact: Operators reading the failure receipt get a misleading explanation and may chase a training-target problem that does not exist.
Sentinel string is returned in the wildcard slot
_normalize_targets (scripts/adapter-serving-compat.py:25-32) returns ([], "missing_or_invalid") for any non-str, non-list-of-str value (e.g. null, a dict, or a mixed list). In assess, the if wildcard: branch at line 61 is evaluated before the if not targets: branch at line 71, so the truthy sentinel triggers the wildcard message: reason "wildcard all-linear training includes Nemotron-H Mamba and routed-MoE targets…" with unsupported_target_modules=["missing_or_invalid"]. The intended branch at lines 71-77 is unreachable for those inputs. Both outcomes are incompatible, so the exit code is right, but the recorded reason is wrong.
Fix by returning None for the wildcard in the invalid case, or checking not targets before the wildcard branch.
| if wildcard: | |
| receipt.update( | |
| compatibility="incompatible", | |
| reason=( | |
| "wildcard all-linear training includes Nemotron-H Mamba and routed-MoE " | |
| "targets that vLLM cannot faithfully represent" | |
| ), | |
| unsupported_target_modules=[wildcard], | |
| ) | |
| return receipt | |
| if not targets: | |
| receipt.update( | |
| compatibility="incompatible", | |
| reason="adapter target_modules is missing or invalid; compatibility must fail closed", | |
| unsupported_target_modules=["missing_or_invalid"], | |
| ) | |
| return receipt | |
| if not targets: | |
| receipt.update( | |
| compatibility="incompatible", | |
| reason="adapter target_modules is missing or invalid; compatibility must fail closed", | |
| unsupported_target_modules=["missing_or_invalid"], | |
| ) | |
| return receipt | |
| if wildcard: | |
| receipt.update( | |
| compatibility="incompatible", | |
| reason=( | |
| "wildcard all-linear training includes Nemotron-H Mamba and routed-MoE " | |
| "targets that vLLM cannot faithfully represent" | |
| ), | |
| unsupported_target_modules=[wildcard], | |
| ) | |
| return receipt |
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _authorized(self): | ||
| # Loopback remains usable for local provider-parity tests. Any | ||
| # network-facing bind is already required to configure a token. | ||
| if not service_token: | ||
| return True | ||
| return bearer_authorized(self.headers.get("authorization"), service_token) |
There was a problem hiding this comment.
🟨 Bearer auth is silently disabled whenever no token is configured
_authorized returns True unconditionally when service_token is unset (scripts/tinker-openai-shim.py:165-170). The startup guard at scripts/tinker-openai-shim.py:70-78 only requires a token when the bind host is not one of the literal loopback strings, and --trusted-proxy-auth waives it entirely. A bind to a non-loopback interface via an alternative spelling (e.g. 0.0.0.0 is caught, but --trusted-proxy-auth used outside Modal, or a host string like :: / a specific LAN IP with the flag set) yields a completely unauthenticated /v1/chat/completions endpoint that spends Tinker API credit and exposes the private checkpoint.
Was this helpful? React with 👍 or 👎 to provide feedback.
| return 500, { | ||
| "error": { | ||
| "message": f"{type_name}: {detail}"[:500], | ||
| "type": "server_error", | ||
| "code": "upstream_error", | ||
| } | ||
| } |
There was a problem hiding this comment.
🟨 Upstream error text is echoed verbatim to clients and to logs
openai_error_response (scripts/tinker_openai_compat.py:38-73) puts the raw exception string (truncated to 500 chars) into the HTTP error body, and log_event("error", ... detail=str(error)[:240]) at scripts/tinker-openai-shim.py:239 writes it to stdout/Modal logs. Tinker client exceptions can embed request URLs, model/checkpoint paths (tinker://...), and occasionally header or payload fragments, so private checkpoint identifiers and internal endpoints can leak to an unauthenticated-in-front-of-proxy caller and into durable logs the module comment claims are prompt-free.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Fixed in 4249551: request body parsing and Content-Length validation are now inside the guarded path, malformed/non-object/missing-messages bodies map to bounded OpenAI-shaped HTTP 400 errors, and provider-free regressions cover each case. |
|
Addressed all additional Devin Review findings in 917e7e1: tool-call responses now emit finish_reason=tool_calls; invalid adapter target_modules produce truthful missing/invalid receipts; trusted proxy mode requires both the explicit shim marker and Modal runtime attestation via MODAL_TASK_ID; upstream exception details are redacted from client bodies and durable logs. Provider-free compatibility, adapter, malformed-request, redaction, and compile checks pass locally. Please re-review the new head. |
1 participant
Summary
Verification
python3 tests/tinker_openai_compat_test.pypython3 scripts/tinker_openai_compat_test.pyNo DEV or holdout evidence is included.