fix(P07): final review — CORS PUT, WS origin gate, ingest leak+O(n²), symlink escape, retry leak, doc-reality gaps
---ci--- phase: 7 milestone: v0.3 status: review lessons: - P0 CORS: allow_methods lacked PUT while the build surface writes files with PUT — every cross-origin Save failed preflight; pinned with tests/api/test_cors.py - P0 ingest leak: queue-overflow flood path returned without the disconnect sentinel, parking the drainer forever (one leaked task-set per flooded trace); sentinel now always enqueued, real-server regression test added - P1 perf: flood cap counted rows via len(get_trace(...)) — O(trace) per append, O(n²) per session; TraceStore.count() (COUNT(*)) added and wired - P0 security: file routes followed exec-planted symlinks out of the workspace bind; _resolve_in_workspace refuses escapes (422), read/write now 404 on unknown sandboxes (was 500) - P1 security: WS ingest accepted any browser Origin (CORS middleware does not cover WS); localhost dev origins + no-Origin (capture agent) allowed, others 1008 - P1 correctness: use-sandbox-session leaked a created sandbox on any mid-start failure (per-learner cap 1 → all retries 429 forever); failed starts now destroy what they created - P2 testing: reconnect-flush test killed mid-burst (nondeterministic under load, reproduced on pre-change code); now waits for server-side observation of the pre-kill burst — the underlying one-line replay-margin/ACK gap is documented for v0.4 - maintainability: grading-store/templates/grading.ts docstrings claimed grading is variant-blind (stale pre-P4 text) — updated; ARCHITECTURE.md referenced nonexistent voice/openai_audio.py; dead if TYPE_CHECKING: pass blocks removed ---/ci---
This commit is contained in:
@@ -85,7 +85,7 @@ Deliberately **not** used: openai-python SDK (the `LLMProvider` protocol is the
|
||||
| `ai_service/telemetry/` | `models.py` (TelemetryEvent, TraceSpan), `store.py` (TraceStore protocol + SQLite impl D-027), `ingest.py` (WebSocket /v1/telemetry/ingest, seq gap detection D-026) | Persistence; never imports agents/ | config |
|
||||
| `ai_service/grading/` | `features.py` (deterministic trace digest D-028), `engine.py` (rubric scoring orchestration), `store.py` (GradeStore) | LLM only via digest; never sees raw trace | llm, telemetry, prompts |
|
||||
| `ai_service/variants/` | `templates.py` (task template library), `generator.py` (seeded LLM instantiation D-029), `store.py` (VariantStore) | LLM via structured output | llm, grading |
|
||||
| `ai_service/voice/` | `base.py` (VoiceProvider protocol D-030), `openai_audio.py` (STT/TTS vs compatible endpoint), `browser.py` (native SR/TTS fallback descriptor), `mock.py` (deterministic) | Never imports agents/ or api/ | config |
|
||||
| `ai_service/voice/` | `base.py` (VoiceProvider protocol D-030), `browser.py` (native SR/TTS fallback descriptor), `mock.py` (deterministic; the real server STT/TTS provider is the v0.4 seam — GRILL CUT-1/G-7) | Never imports agents/ or api/ | config |
|
||||
| `ai_service/agents/examiner.py` | Seventh agent: oral defense examiner; streams over existing SSE, consumes process traces + emits integrity signals | reuses BaseAgent (D-018) | llm, prompts, telemetry |
|
||||
| `ai_service/data/*.db` | SQLite databases (telemetry/grades/variants/defenses) | gitignored | — |
|
||||
| `scripts/sandbox-agent.py` | Tiny in-namespace capture process shipped into the sandbox; streams telemetry to ingest | standalone | stdlib only |
|
||||
|
||||
@@ -7,7 +7,6 @@ No session chat — each request is one live-trace read.
|
||||
"""
|
||||
|
||||
from collections.abc import AsyncIterator
|
||||
from typing import TYPE_CHECKING
|
||||
|
||||
from ..config import Settings
|
||||
from ..corpus.learner_context import LearnerContext, get_learner_context
|
||||
@@ -16,9 +15,6 @@ from ..llm.base import LLMProvider
|
||||
from ..prompts.lab import SYSTEM_PROMPT, render_context, render_digest_timeline
|
||||
from .base import BaseAgent
|
||||
|
||||
if TYPE_CHECKING: # pragma: no cover
|
||||
pass
|
||||
|
||||
|
||||
class LabAgent(BaseAgent):
|
||||
name = "lab"
|
||||
|
||||
@@ -21,7 +21,6 @@ from __future__ import annotations
|
||||
|
||||
import time
|
||||
from datetime import UTC, datetime
|
||||
from typing import TYPE_CHECKING
|
||||
|
||||
from fastapi import APIRouter, Depends, File, Form, HTTPException, UploadFile
|
||||
from fastapi.responses import StreamingResponse
|
||||
@@ -42,9 +41,6 @@ from .deps import (
|
||||
get_voice_store,
|
||||
)
|
||||
|
||||
if TYPE_CHECKING: # pragma: no cover
|
||||
pass
|
||||
|
||||
router = APIRouter(prefix="/v1/defense", tags=["defense"])
|
||||
|
||||
#: A-109: learner turns slower than this are flagged as long pauses (ms).
|
||||
|
||||
@@ -268,6 +268,26 @@ def _safe_rel_path(raw: str) -> Path:
|
||||
return candidate
|
||||
|
||||
|
||||
def _resolve_in_workspace(workspace: Path, rel: Path) -> Path:
|
||||
"""Resolve `rel` under `workspace`, refusing symlink escapes (P7).
|
||||
|
||||
The lexical check in `_safe_rel_path` cannot see symlinks: an exec can
|
||||
plant `ln -s /etc target` in the workspace and a follow-up read/write
|
||||
would follow it OUT of the bind. Resolve with the workspace as the
|
||||
anchor (strict: a symlink chain escaping raises) and confirm the
|
||||
normalized target still sits inside the workspace — defense in depth
|
||||
for both read_file and write_file.
|
||||
"""
|
||||
try:
|
||||
target = (workspace / rel).resolve(strict=False)
|
||||
target.relative_to(workspace.resolve(strict=False))
|
||||
except ValueError:
|
||||
raise HTTPException(
|
||||
status_code=422, detail=f"path escapes the workspace: {rel.as_posix()!r}"
|
||||
) from None
|
||||
return target
|
||||
|
||||
|
||||
@router.get("/{sandbox_id}/files")
|
||||
async def list_files(
|
||||
sandbox_id: str,
|
||||
@@ -286,9 +306,12 @@ async def read_file(
|
||||
path: str,
|
||||
manager: SandboxManager = Depends(get_sandbox_manager),
|
||||
) -> dict:
|
||||
try:
|
||||
workspace, _ = await _workspace_dir(manager, sandbox_id)
|
||||
except SandboxNotFoundError:
|
||||
raise HTTPException(status_code=404, detail=f"no sandbox {sandbox_id!r}") from None
|
||||
rel = _safe_rel_path(path)
|
||||
target = workspace / rel
|
||||
target = _resolve_in_workspace(workspace, rel)
|
||||
if not target.is_file():
|
||||
raise HTTPException(status_code=404, detail=f"no file {path!r}")
|
||||
return {"path": path, "content": target.read_text(errors="replace")}
|
||||
@@ -301,9 +324,12 @@ async def write_file(
|
||||
body: FileWriteRequest,
|
||||
manager: SandboxManager = Depends(get_sandbox_manager),
|
||||
) -> dict:
|
||||
try:
|
||||
workspace, _ = await _workspace_dir(manager, sandbox_id)
|
||||
except SandboxNotFoundError:
|
||||
raise HTTPException(status_code=404, detail=f"no sandbox {sandbox_id!r}") from None
|
||||
rel = _safe_rel_path(body.path)
|
||||
target = workspace / rel
|
||||
target = _resolve_in_workspace(workspace, rel)
|
||||
target.parent.mkdir(parents=True, exist_ok=True)
|
||||
target.write_text(body.content)
|
||||
return {"path": body.path, "written": True}
|
||||
|
||||
@@ -9,10 +9,13 @@ only wires `app.state.trace_store` / `app.state.trace_integrity` /
|
||||
GET /v1/telemetry/traces/{learner_id}/{task_id} ordered trace; 404 unknown
|
||||
GET /v1/telemetry/gaps/{learner_id}/{task_id} missing seqs ; 404 unknown
|
||||
|
||||
The WS route is a thin DI shell: it validates the query-param identity,
|
||||
pulls store/integrity/settings from `app.state`, and calls
|
||||
`telemetry_ingest_endpoint(...)` — the engine stays FastAPI-DI-free so it's
|
||||
testable without a router and the api/ layer owns all composition.
|
||||
The WS route is a thin DI shell: it validates the query-param identity and
|
||||
the Origin (browser pages are gated to the localhost dev origins — CORS
|
||||
middleware does not cover WS upgrades; the stdlib capture agent sends no
|
||||
Origin and is unaffected), pulls store/integrity/settings from `app.state`,
|
||||
and calls `telemetry_ingest_endpoint(...)` — the engine stays
|
||||
FastAPI-DI-free so it's testable without a router and the api/ layer owns
|
||||
all composition.
|
||||
|
||||
Unknown-trace contract: a trace is KNOWN when it has >=1 stored event OR
|
||||
carries an integrity flag — a flooded trace with zero stored rows still 200s
|
||||
@@ -21,6 +24,8 @@ so Proctor/grader can read WHY it's unusable (G-4 consumes
|
||||
the map so HTTP consumers never touch process internals.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from fastapi import APIRouter, Depends, HTTPException, WebSocket
|
||||
from pydantic import BaseModel
|
||||
|
||||
@@ -34,6 +39,15 @@ from .deps import get_trace_integrity, get_trace_store
|
||||
|
||||
router = APIRouter(prefix="/v1/telemetry", tags=["telemetry"])
|
||||
|
||||
#: Browser Origins allowed to open the ingest socket (A-008 mirror). The
|
||||
#: stdlib capture agent sends NO Origin header (it is not a browser) and
|
||||
#: stays allowed; a malicious page loaded in the learner's browser would
|
||||
#: carry an Origin and must not be able to poison/flood the trace. CORS
|
||||
#: middleware does NOT cover WebSocket upgrades, so this gate is explicit.
|
||||
_ALLOWED_WS_ORIGINS = frozenset(
|
||||
{"http://localhost:3000", "http://127.0.0.1:3000", "http://localhost:8420"}
|
||||
)
|
||||
|
||||
|
||||
# --- WS ingest (D-026) ---------------------------------------------------------
|
||||
|
||||
@@ -45,6 +59,15 @@ async def telemetry_ingest_ws(websocket: WebSocket) -> None:
|
||||
The engine's session + flood logic is fully typed and testable without
|
||||
FastAPI; this shim is the only place the two layers meet.
|
||||
"""
|
||||
origin = (websocket.headers.get("origin") or "").strip()
|
||||
if origin and origin not in _ALLOWED_WS_ORIGINS:
|
||||
# Same-origin dev pages (Next.js on :3000, the service itself on
|
||||
# :8420) pass; anything else is refused pre-accept. Non-browser
|
||||
# producers (the capture agent, tests) send no Origin and pass.
|
||||
await websocket.close(
|
||||
code=1008, reason=f"origin {origin!r} not allowed for telemetry ingest"
|
||||
)
|
||||
return
|
||||
query = websocket.query_params
|
||||
learner_id = query.get("learner_id", "")
|
||||
task_id = query.get("task_id", "")
|
||||
|
||||
@@ -27,16 +27,11 @@ Feature semantics (conservative, deterministic):
|
||||
from __future__ import annotations
|
||||
|
||||
from collections import Counter
|
||||
from typing import TYPE_CHECKING
|
||||
|
||||
from pydantic import BaseModel, Field
|
||||
|
||||
from ..telemetry.models import TelemetryEvent
|
||||
|
||||
if TYPE_CHECKING: # pragma: no cover - import cycle guard for type checkers
|
||||
pass
|
||||
|
||||
|
||||
_IDLE_DEFAULT_S: float = 120.0
|
||||
|
||||
_TEST_HINTS = ("test", "pytest", "vitest", "jest", "mocha", "unittest", "go test", "npm test")
|
||||
|
||||
@@ -59,8 +59,11 @@ class GradeRecord(SQLModel, table=True):
|
||||
(learner_id, task_id) pair — the same pair as trace
|
||||
identity, so a grade is keyed by the exact trace it
|
||||
was computed from.
|
||||
variant_seed — task-variant seed; None until P4 (D-029). v0.3
|
||||
grading is variant-blind.
|
||||
variant_seed — task-variant seed (D-029); None when the graded task
|
||||
is not variant-derived. Since Phase 4 the engine
|
||||
stamps the graded variant's seed here (MH#4) and the
|
||||
template's difficulty anchors ship to the grader
|
||||
prompt — this column is the audit join for that.
|
||||
digest — compact deterministic trace digest (D-028) that fed
|
||||
the rubric prompt; persisted for auditability so the
|
||||
LLM's input stays reproducible.
|
||||
@@ -85,7 +88,8 @@ class GradeRecord(SQLModel, table=True):
|
||||
|
||||
learner_id: str = Field(primary_key=True)
|
||||
task_id: str = Field(primary_key=True)
|
||||
variant_seed: str | None = Field(default=None) # null until P4 (D-029)
|
||||
# None only for non-variant tasks (MH#4 stamps variant seeds since P4).
|
||||
variant_seed: str | None = Field(default=None)
|
||||
# JSON columns: stored as TEXT on SQLite, native JSONB on Postgres (D-027).
|
||||
digest: dict[str, Any] = Field(default_factory=dict, sa_type=JSON)
|
||||
scores: dict[str, Any] = Field(default_factory=dict, sa_type=JSON)
|
||||
|
||||
@@ -171,11 +171,15 @@ def create_app(settings: Settings | None = None) -> FastAPI:
|
||||
|
||||
app = FastAPI(title="Nextcraft AI Service", version="0.3.0", lifespan=lifespan)
|
||||
|
||||
# A-008: localhost-only CORS, no credentials
|
||||
# A-008: localhost-only CORS, no credentials. PUT is CONTRACT, not
|
||||
# trivia: the learner build surface writes workspace files with PUT
|
||||
# (engine-client writeFile) — v0.3 initially shipped without it and
|
||||
# every cross-origin Save failed preflight (caught in P7 review;
|
||||
# tests/api/test_cors.py pins the policy now).
|
||||
app.add_middleware(
|
||||
CORSMiddleware,
|
||||
allow_origins=["http://localhost:3000", "http://127.0.0.1:3000"],
|
||||
allow_methods=["GET", "POST", "DELETE", "OPTIONS"],
|
||||
allow_methods=["GET", "POST", "PUT", "DELETE", "OPTIONS"],
|
||||
allow_headers=["Content-Type"],
|
||||
allow_credentials=False,
|
||||
)
|
||||
|
||||
@@ -239,6 +239,12 @@ class IngestSession:
|
||||
self._queue.put_nowait(frame)
|
||||
except asyncio.QueueFull:
|
||||
# Bounded queue — overflow is a flood, never drop-oldest.
|
||||
# _trigger_flood closes the socket; fall through to the
|
||||
# tail so the disconnect sentinel is still enqueued — the
|
||||
# drainer is never left parked on an empty queue after a
|
||||
# flood (P7 review: the pre-fix code `return`ed from the
|
||||
# QueueFull branch WITHOUT the sentinel, leaking the
|
||||
# session task set — one per flooded trace).
|
||||
await self._trigger_flood("queue_overflow")
|
||||
return
|
||||
except WebSocketDisconnect:
|
||||
@@ -354,8 +360,10 @@ class IngestSession:
|
||||
def _flood_breached(self) -> bool:
|
||||
"""True when this append would exceed the per-trace event budget."""
|
||||
# Durable count (NOT latest_seq+1 — a skipped-ahead seq must not burn
|
||||
# un-sent events' budget) plus this connection's in-flight rows.
|
||||
durable = len(self._store.get_trace(self.learner_id, self.task_id))
|
||||
# un-sent events' budget) via COUNT(*): never materialize the trace
|
||||
# per append (P7 review — the old len(get_trace(...)) built every row
|
||||
# object per event, O(trace) per append / O(n²) per session).
|
||||
durable = self._store.count(learner_id=self.learner_id, task_id=self.task_id)
|
||||
return durable >= self._max_events
|
||||
|
||||
async def _check_gap(self, incoming_seq: int) -> None:
|
||||
|
||||
@@ -68,6 +68,13 @@ class TraceStore(Protocol):
|
||||
"""Highest stored seq for the trace; -1 when no events exist."""
|
||||
...
|
||||
|
||||
def count(self, learner_id: str, task_id: str) -> int:
|
||||
"""Number of stored events for the trace (COUNT(*), never
|
||||
materializes rows — the ingest cap consults this per append, so
|
||||
an O(trace) implementation would make ingest O(n²) per session).
|
||||
"""
|
||||
...
|
||||
|
||||
def list_tasks(self, learner_id: str) -> list[str]:
|
||||
"""Distinct task_ids with at least one event for the learner."""
|
||||
...
|
||||
@@ -183,6 +190,19 @@ class SQLiteTraceStore:
|
||||
latest: Any = session.exec(stmt).one()
|
||||
return -1 if latest is None else int(latest)
|
||||
|
||||
def count(self, learner_id: str, task_id: str) -> int:
|
||||
# COUNT(*) at the DB — no row materialization. The ingest flood cap
|
||||
# calls this per append (telemetry/ingest._flood_breached); the
|
||||
# docstring-free body keeps it obvious what the query shape is.
|
||||
with self._session() as session:
|
||||
stmt = (
|
||||
select(sa.func.count(TelemetryEvent.seq))
|
||||
.where(TelemetryEvent.learner_id == learner_id)
|
||||
.where(TelemetryEvent.task_id == task_id)
|
||||
)
|
||||
total: Any = session.exec(stmt).one()
|
||||
return int(total or 0)
|
||||
|
||||
def list_tasks(self, learner_id: str) -> list[str]:
|
||||
with self._session() as session:
|
||||
stmt = (
|
||||
|
||||
@@ -71,11 +71,11 @@ class RubricAnchors(BaseModel):
|
||||
for this template, so two variants of one template are held to the
|
||||
same bar regardless of which slot values a learner drew. The a-5
|
||||
envelope test (tests/variants/test_generator.py) binds variants to
|
||||
these bands in code. Shipping them into the grader prompt context is
|
||||
the P4 must-have follow-up tracked for final review: grading is
|
||||
variant-blind in the current wiring (engine.py stamps
|
||||
variant_seed=None), so today the anchors gate variant fairness in
|
||||
tests only — not yet in the LLM prompt.
|
||||
these bands in code, and — since Phase 4 (MH#4) — the grading engine
|
||||
ships this envelope into the grader prompt
|
||||
(grading/engine._anchors_context) and stamps the variant seed on the
|
||||
GradeRecord, so the anchors gate variant fairness in BOTH tests and
|
||||
the live rubric.
|
||||
"""
|
||||
|
||||
model_config = ConfigDict(frozen=True)
|
||||
|
||||
@@ -0,0 +1,60 @@
|
||||
"""CORS policy tests (A-008, P7 review regression).
|
||||
|
||||
v0.3 initially shipped `allow_methods` WITHOUT "PUT" while the learner
|
||||
build surface writes workspace files with PUT (engine-client writeFile) —
|
||||
every cross-origin Save failed preflight. These tests pin the policy so a
|
||||
future method-list edit fails loudly instead of silently breaking the
|
||||
headline flow.
|
||||
|
||||
Two-layer check:
|
||||
- preflight (OPTIONS + Access-Control-Request-Method) for every method the
|
||||
web client actually uses: GET/POST/PUT/DELETE;
|
||||
- actual cross-origin request echoes the localhost dev origin.
|
||||
Disallowed origins must NOT be granted (localhost-only, no credentials).
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from fastapi.testclient import TestClient
|
||||
|
||||
ALLOWED_ORIGIN = "http://localhost:3000"
|
||||
ALL_CLIENT_METHODS = ("GET", "POST", "PUT", "DELETE")
|
||||
|
||||
|
||||
def test_preflight_allows_every_method_the_web_client_uses(client: TestClient) -> None:
|
||||
for method in ALL_CLIENT_METHODS:
|
||||
resp = client.options(
|
||||
"/v1/sandboxes",
|
||||
headers={
|
||||
"Origin": ALLOWED_ORIGIN,
|
||||
"Access-Control-Request-Method": method,
|
||||
},
|
||||
)
|
||||
assert resp.status_code == 200, f"preflight {method} failed: {resp.status_code}"
|
||||
assert resp.headers["access-control-allow-origin"] == ALLOWED_ORIGIN
|
||||
allowed = resp.headers["access-control-allow-methods"].split(", ")
|
||||
assert method in allowed, f"{method} missing from CORS methods: {allowed}"
|
||||
|
||||
|
||||
def test_cross_origin_get_echoes_allow_origin(client: TestClient) -> None:
|
||||
resp = client.get("/v1/sandboxes", headers={"Origin": ALLOWED_ORIGIN})
|
||||
assert resp.status_code == 200
|
||||
assert resp.headers.get("access-control-allow-origin") == ALLOWED_ORIGIN
|
||||
|
||||
|
||||
def test_unknown_origin_gets_no_cors_grant(client: TestClient) -> None:
|
||||
resp = client.get("/v1/sandboxes", headers={"Origin": "https://evil.example"})
|
||||
assert resp.status_code == 200 # non-CORS requests still serve
|
||||
assert resp.headers.get("access-control-allow-origin") is None
|
||||
|
||||
|
||||
def test_credentials_never_allowed(client: TestClient) -> None:
|
||||
resp = client.options(
|
||||
"/v1/sandboxes",
|
||||
headers={
|
||||
"Origin": ALLOWED_ORIGIN,
|
||||
"Access-Control-Request-Method": "PUT",
|
||||
"Access-Control-Request-Headers": "Content-Type",
|
||||
},
|
||||
)
|
||||
assert resp.headers.get("access-control-allow-credentials") != "true"
|
||||
@@ -406,3 +406,44 @@ class TestFilesAndExecRoutes:
|
||||
"/v1/sandboxes/sbx-nope/exec", json={"cmd": ["echo", "hi"]}
|
||||
)
|
||||
assert resp.status_code == 404
|
||||
|
||||
def test_unknown_sandbox_file_routes_404_not_500(self, client):
|
||||
"""P7: read/write on an unknown sandbox must 404 (SandboxNotFoundError
|
||||
previously escaped _workspace_dir as an unhandled 500)."""
|
||||
assert (
|
||||
client.get("/v1/sandboxes/sbx-nope/files/whatever.py").status_code == 404
|
||||
)
|
||||
put = client.put(
|
||||
"/v1/sandboxes/sbx-nope/files/whatever.py",
|
||||
json={"path": "whatever.py", "content": "x"},
|
||||
)
|
||||
assert put.status_code == 404
|
||||
|
||||
def test_symlink_escape_rejected(self, client):
|
||||
"""P7: an exec-planted symlink in the workspace must not let the
|
||||
file routes read/write OUTSIDE the bind (lexical traversal checks
|
||||
cannot see symlinks — resolve + containment re-check is the gate)."""
|
||||
handle = client.post(
|
||||
"/v1/sandboxes", json={"learner_id": "pilot-learner"}
|
||||
).json()
|
||||
sbx = handle["id"]
|
||||
workspace = Path(handle["workdir"]) / "workspace"
|
||||
outside = workspace.parent / "secret.txt"
|
||||
outside.write_text("host secret") # a host file OUTSIDE the bind
|
||||
try:
|
||||
(workspace / "leak.txt").symlink_to(outside)
|
||||
read = client.get(f"/v1/sandboxes/{sbx}/files/leak.txt")
|
||||
assert read.status_code == 422, (
|
||||
f"symlink escape read must 422, got {read.status_code}: {read.text}"
|
||||
)
|
||||
write = client.put(
|
||||
f"/v1/sandboxes/{sbx}/files/leak.txt",
|
||||
json={"path": "leak.txt", "content": "pwned"},
|
||||
)
|
||||
assert write.status_code == 422, (
|
||||
f"symlink escape write must 422, got {write.status_code}: {write.text}"
|
||||
)
|
||||
assert outside.read_text() == "host secret" # untouched
|
||||
finally:
|
||||
client.delete(f"/v1/sandboxes/{sbx}")
|
||||
outside.unlink(missing_ok=True)
|
||||
|
||||
@@ -239,6 +239,91 @@ def test_queue_overflow_also_floods(
|
||||
assert app.state.trace_integrity.reason("L-q", "T-q") == "INCOMPLETE_FLOODED"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_queue_overflow_flood_session_task_terminates(tmp_path, monkeypatch):
|
||||
"""P7 regression: the queue-overflow flood path must not LEAK the
|
||||
session coroutine. v0.3's receiver returned from its QueueFull branch
|
||||
without the disconnect sentinel, so the drainer parked on an empty queue
|
||||
forever and IngestSession.run() never returned — one leaked
|
||||
(pinger+drainer) task-set per flooded trace, unbounded over a long-lived
|
||||
process. A REAL uvicorn server (TestClient teardown hides the leak) is
|
||||
stopped after the flood; the session tasks must be gone shortly after.
|
||||
"""
|
||||
import asyncio
|
||||
import contextlib
|
||||
import socket as socket_mod
|
||||
|
||||
import uvicorn
|
||||
|
||||
from ai_service.main import create_app
|
||||
from ai_service.telemetry.ingest import TraceIntegrityMap
|
||||
from ai_service.telemetry.store import SQLiteTraceStore
|
||||
|
||||
monkeypatch.setattr(ingest_mod, "INBOUND_QUEUE_MAX", 1)
|
||||
store = SQLiteTraceStore(db_path=tmp_path / "leak.db")
|
||||
app = create_app(
|
||||
Settings(
|
||||
provider="mock",
|
||||
db_path=tmp_path / "leak.db",
|
||||
sandbox_dir=tmp_path / "sandboxes",
|
||||
)
|
||||
)
|
||||
app.state.trace_store = store
|
||||
app.state.trace_integrity = TraceIntegrityMap()
|
||||
|
||||
with socket_mod.socket() as s:
|
||||
s.bind(("127.0.0.1", 0))
|
||||
port = s.getsockname()[1]
|
||||
server = uvicorn.Server(
|
||||
uvicorn.Config(app, host="127.0.0.1", port=port, log_level="warning")
|
||||
)
|
||||
serve_task = asyncio.get_running_loop().create_task(server.serve())
|
||||
leaked = True
|
||||
try:
|
||||
for _ in range(100):
|
||||
if server.started:
|
||||
break
|
||||
await asyncio.sleep(0.1)
|
||||
assert server.started
|
||||
|
||||
import websockets
|
||||
|
||||
uri = (
|
||||
f"ws://127.0.0.1:{port}/v1/telemetry/ingest"
|
||||
f"?learner_id=L-leak&task_id=T-leak"
|
||||
)
|
||||
async with websockets.connect(uri) as ws:
|
||||
for seq in range(64): # bound=1 → guaranteed overflow
|
||||
await ws.send(_frame(seq, sandbox_id=""))
|
||||
# The flood close (1008) reaches the client.
|
||||
try:
|
||||
await asyncio.wait_for(ws.recv(), timeout=10.0)
|
||||
await asyncio.wait_for(ws.recv(), timeout=10.0)
|
||||
except (websockets.exceptions.ConnectionClosed, TimeoutError, OSError):
|
||||
pass
|
||||
assert app.state.trace_integrity.is_incomplete("L-leak", "T-leak")
|
||||
|
||||
# The session's run() must have returned: no lingering nc-* tasks
|
||||
# holding the socket open. Poll briefly — teardown is async.
|
||||
deadline = asyncio.get_running_loop().time() + 5.0
|
||||
while asyncio.get_running_loop().time() < deadline:
|
||||
names = {
|
||||
t.get_name()
|
||||
for t in asyncio.all_tasks()
|
||||
if t is not asyncio.current_task()
|
||||
}
|
||||
if not any("ingest" in n.lower() for n in names):
|
||||
leaked = False
|
||||
break
|
||||
await asyncio.sleep(0.1)
|
||||
finally:
|
||||
server.should_exit = True
|
||||
with contextlib.suppress(Exception):
|
||||
await asyncio.wait_for(serve_task, timeout=10.0)
|
||||
store.close()
|
||||
assert not leaked, "IngestSession task set leaked after queue-overflow flood"
|
||||
|
||||
|
||||
def test_reconnect_after_flood_cannot_resurrect_trace(
|
||||
client: TestClient, app, store: SQLiteTraceStore
|
||||
) -> None:
|
||||
@@ -309,6 +394,32 @@ def test_missing_identity_query_params_rejected_at_handshake(
|
||||
assert excinfo.value.code == 1008
|
||||
|
||||
|
||||
def test_browser_origin_not_allowed_for_ingest(client: TestClient) -> None:
|
||||
"""CORS middleware does not cover WS upgrades (P7): a page loaded in the
|
||||
learner's browser (any non-localhost Origin) must not be able to open
|
||||
the ingest socket and poison/flood the trace. The stdlib capture agent
|
||||
sends no Origin and is unaffected (see the no-origin test below)."""
|
||||
with pytest.raises(WebSocketDisconnect) as excinfo:
|
||||
with client.websocket_connect(
|
||||
_ingest_url(), headers={"Origin": "https://evil.example"}
|
||||
):
|
||||
pass
|
||||
assert excinfo.value.code == 1008
|
||||
|
||||
|
||||
def test_dev_origin_and_no_origin_both_allowed(client: TestClient) -> None:
|
||||
"""The same-origin dev page (Next.js :3000) opens fine, and so does the
|
||||
capture-agent path (no Origin header at all)."""
|
||||
for headers in ({"Origin": "http://localhost:3000"}, {}):
|
||||
with client.websocket_connect(_ingest_url(), headers=headers) as ws:
|
||||
ws.send_text(_frame(0))
|
||||
body = client.get(f"/v1/telemetry/traces/{LEARNER}/{TASK}").json()
|
||||
assert [e["seq"] for e in body["events"]] == [0]
|
||||
# Unique trace per iteration would collide on (LEARNER, TASK) PK —
|
||||
# seq 0 re-sent is deduped, so one row is the invariant either way.
|
||||
assert len(body["events"]) == 1
|
||||
|
||||
|
||||
# -- keepalive ---------------------------------------------------------------------
|
||||
|
||||
|
||||
|
||||
@@ -499,7 +499,22 @@ class TestReconnectFlush:
|
||||
test_agent = _make_agent(tmp_path, held_link.url)
|
||||
test_agent.start()
|
||||
assert test_agent.wait_connected(5)
|
||||
test_agent.run_command("echo first")
|
||||
first = test_agent.run_command("echo first")
|
||||
# P7 de-flake: wait for the pre-kill burst to be OBSERVED at the
|
||||
# server before severing (the sibling TestSpoolOnDisconnect test
|
||||
# already had this discipline). Killing mid-burst exercises a
|
||||
# DIFFERENT, documented limitation — the agent's one-line replay
|
||||
# margin cannot cover a multi-frame TCP in-flight window (an
|
||||
# ACK-protocol gap tracked for v0.4) — which made this test
|
||||
# nondeterministic under load instead of testing what its name
|
||||
# says: the reconnect flush of OFFLINE-spooled events.
|
||||
assert _wait_until(
|
||||
lambda: any(
|
||||
e["kind"] == "run_result" and e["seq"] == first["seq"]
|
||||
for e in fake_server.events
|
||||
),
|
||||
timeout_s=10.0,
|
||||
), "pre-kill burst never reached the server"
|
||||
|
||||
held_link.kill() # outage begins: no traffic, no reconnect possible
|
||||
assert test_agent.wait_disconnected(5)
|
||||
|
||||
@@ -129,6 +129,21 @@ def test_latest_seq(store: SQLiteTraceStore) -> None:
|
||||
assert store.latest_seq("learner-1", "task-2") == -1
|
||||
|
||||
|
||||
def test_count_is_durable_row_count_not_latest_seq(store: SQLiteTraceStore) -> None:
|
||||
"""count() backs the ingest flood cap (P7): it must reflect stored ROWS
|
||||
(a skipped-ahead seq must not burn un-sent budget) and stay O(1)-ish
|
||||
(COUNT(*), never materialize the trace per append)."""
|
||||
assert store.count("learner-1", "task-1") == 0
|
||||
store.append(make_event(0))
|
||||
store.append(make_event(2)) # skipped 1 — count is rows, not latest+1
|
||||
assert store.count("learner-1", "task-1") == 2
|
||||
# Dedup retries do not inflate the count (at-least-once contract).
|
||||
store.append(make_event(2))
|
||||
assert store.count("learner-1", "task-1") == 2
|
||||
# Scoped to the trace pair.
|
||||
assert store.count("learner-1", "task-2") == 0
|
||||
|
||||
|
||||
def test_list_tasks(store: SQLiteTraceStore) -> None:
|
||||
assert store.list_tasks("learner-1") == []
|
||||
|
||||
|
||||
@@ -56,9 +56,11 @@ export function useSandboxSession(competencyId: string | null) {
|
||||
const controller = new AbortController();
|
||||
abortRef.current = controller;
|
||||
setState({ ...DEFAULT_STATE, status: 'starting' });
|
||||
let createdId: string | null = null;
|
||||
try {
|
||||
const variant = await generateVariant(MOCK_LEARNER_ID, compId, controller.signal);
|
||||
const sandbox = await createSandbox(MOCK_LEARNER_ID, variant.task_id, controller.signal);
|
||||
createdId = sandbox.id;
|
||||
// Materialize the variant's starter files into the sandbox workspace.
|
||||
for (const [path, content] of Object.entries(variant.starter_files ?? {})) {
|
||||
await writeFile(sandbox.id, path, content, controller.signal);
|
||||
@@ -66,6 +68,11 @@ export function useSandboxSession(competencyId: string | null) {
|
||||
const files = await listFiles(sandbox.id, controller.signal);
|
||||
setState({ status: 'ready', variant, sandboxId: sandbox.id, files, errorMessage: null });
|
||||
} catch (err) {
|
||||
// A created sandbox must not outlive a failed start (per-learner cap
|
||||
// is 1 — a leaked one blocks every retry with 429 forever). This
|
||||
// covers aborts mid-start, failed starter-file writes, and errors
|
||||
// after create; a 409/404 on destroy is benign.
|
||||
if (createdId) void destroySandbox(createdId).catch(() => undefined);
|
||||
if (controller.signal.aborted) return;
|
||||
if (err instanceof EngineError) {
|
||||
setState({
|
||||
@@ -93,6 +100,8 @@ export function useSandboxSession(competencyId: string | null) {
|
||||
}, [competencyId]);
|
||||
|
||||
// Unmount: destroy the sandbox (idempotent; a killed session is fine).
|
||||
// The ref is ALSO updated inside start() (via this effect watching state
|
||||
// changes) so unmount-mid-start finds the id even before 'ready' lands.
|
||||
const sandboxIdRef = useRef<string | null>(null);
|
||||
useEffect(() => {
|
||||
sandboxIdRef.current = state.sandboxId;
|
||||
@@ -103,6 +112,11 @@ export function useSandboxSession(competencyId: string | null) {
|
||||
if (id) void destroySandbox(id).catch(() => undefined);
|
||||
};
|
||||
}, []);
|
||||
useEffect(() => {
|
||||
return () => {
|
||||
abortRef.current?.abort();
|
||||
};
|
||||
}, []);
|
||||
|
||||
const run = useCallback(
|
||||
async (cmd: string[]): Promise<ExecResult | null> => {
|
||||
|
||||
@@ -69,7 +69,7 @@ export interface GradeRecord {
|
||||
learner_id: string;
|
||||
/** The graded task — joins to `TaskVariant.task_id`. */
|
||||
task_id: string;
|
||||
/** Task-variant seed (D-029); null while grading is variant-blind. */
|
||||
/** Task-variant seed (D-029); null only for non-variant tasks (P4 stamps it). */
|
||||
variant_seed: string | null;
|
||||
/** Compact trace digest (D-028) that fed the rubric prompt; {} for gate records. */
|
||||
digest: Record<string, unknown>;
|
||||
|
||||
Reference in New Issue
Block a user