From 134ec9a68e67ca6b9382d2a5a0615511bc87d291 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 20 Sep 2026 00:09:52 +0200 Subject: [PATCH] =?UTF-8?q?fix(diagnostics):=20deep-review=20round=201=20?= =?UTF-8?q?=E2=80=94=20a=20send=20that=20cannot=20race,=20lose=20or=20repe?= =?UTF-8?q?at=20itself?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The send, the draft and the screenshots take the session's row lock, so an autosave or a paste during a send waits for it and then sees it, and two sends cannot open two issues. A save keeps the delivery's progress; a failed edit that shows the screenshots is finished by the retry; an answer that is not the tracker's API (a redirect, a proxy page) is refused, not a crash that loses the issue's record. Only an attempt that opened an issue counts toward the pace. Screenshots: deleting one after the send is refused, a user's storage is capped across sessions, new sessions and zip exports are paced, images are stored uncompressed in the zip. One liveness rule, one session view, one image type table. Logging: a session's elevation is tracked apart from the user's own, so the browser's own requests (no header) no longer lower the floor a session needs; Stop ends it at once. Composer: an image pasted anywhere is taken; what is still unsaved is saved on close, and "saved" is shown only when it is; the draft is held while it is sent; a failed screenshot read says so instead of reading as none; focus moves to what stays; the preview follows the text size. Pill: the step prompt and a running Stop survive the pill moving between hosts, the prompt keeps the dialog's Tab trap, the toast region lets clicks through, other tabs hear of a Start or Stop, the last seconds are sent before the cap, a capture with no frame is an error. --- backend/app/debugsession.py | 105 ++++++-- backend/app/issues/bundle.py | 9 +- backend/app/issues/gitea.py | 87 +++--- backend/app/logs.py | 39 ++- backend/app/routes/debug_sessions.py | 83 ++++-- backend/app/routes/me.py | 4 +- backend/tests/test_debug_sessions.py | 175 +++++++++++- frontend/src/components/AppCommandPalette.tsx | 13 +- .../src/components/DebugRecorder.dom.test.tsx | 89 +++++- frontend/src/components/DebugRecorder.tsx | 39 ++- .../src/components/IssueComposer.dom.test.tsx | 114 +++++++- frontend/src/components/IssueComposer.tsx | 253 +++++++++++++----- .../src/components/ui/Toaster.dom.test.tsx | 8 + frontend/src/components/ui/Toaster.tsx | 7 +- frontend/src/components/useDebugRecording.ts | 45 +++- frontend/src/i18n/locales/en/errors.json | 5 +- frontend/src/i18n/locales/en/recorder.json | 4 +- frontend/src/i18n/locales/hu/errors.json | 5 +- frontend/src/i18n/locales/hu/recorder.json | 4 +- frontend/src/lib/captureTab.dom.test.ts | 55 ++++ frontend/src/lib/captureTab.ts | 21 +- frontend/src/lib/debugRecorder.ts | 28 ++ .../src/lib/diagnostics.session.dom.test.ts | 10 + frontend/src/lib/diagnostics.ts | 10 + frontend/src/lib/queryKeys.ts | 3 + 25 files changed, 1007 insertions(+), 208 deletions(-) create mode 100644 frontend/src/lib/captureTab.dom.test.ts diff --git a/backend/app/debugsession.py b/backend/app/debugsession.py index b7dabaff..3f114733 100644 --- a/backend/app/debugsession.py +++ b/backend/app/debugsession.py @@ -32,6 +32,12 @@ HEADER = "x-debug-session" MAX_MINUTES = diagnostics.SESSION_MINUTES SCREENSHOT_MAX_BYTES = 10 * 1024 * 1024 SCREENSHOTS_MAX = 8 +# What one user may keep in screenshots across ALL their sessions (until retention takes them): the +# per-session cap alone would let Start → 8 images → Stop repeat without end. +SCREENSHOT_USER_MAX_BYTES = 100 * 1024 * 1024 +# The screenshot types kept, and the extension each is exported and sent under — one table, so an +# accepted type always has a name. +IMAGE_TYPES = {"image/png": "png", "image/jpeg": "jpg", "image/webp": "webp"} # Rows an issue can carry. A 60-minute session at Teljes is a few thousand rows; past this the # bundle says how many it left out rather than growing without bound. BUNDLE_MAX = 5_000 @@ -48,15 +54,16 @@ class EndReason(StrEnum): EXPIRED = "expired" -def _is_live(session: DebugSession, now: datetime) -> bool: - return session.ended_at is None and session.until > now +def is_live(session: DebugSession, now: datetime | None = None) -> bool: + """Open and within its cap — THE liveness rule: an open session past its cap is over.""" + return session.ended_at is None and session.until > (now or datetime.now(timezone.utc)) def open_session(db: Session, user: User) -> DebugSession | None: """The user's live session, or None. Read-only: one past its cap is over and reads as None; `start` and the hourly sweep are what close it.""" session = _unclosed(db, user) - if session is None or not _is_live(session, datetime.now(timezone.utc)): + if session is None or not is_live(session, datetime.now(timezone.utc)): return None return session @@ -71,11 +78,11 @@ def start(db: Session, user: User, level: SessionLevel) -> DebugSession: """Open a session, or return the live one — a second tab pressing Start joins the recording rather than ending it. One past its cap is closed first: the one-open-per-user index would refuse the new row beside it. Flushes; the caller commits. Two concurrent Starts can still both - get here: the index refuses the second, and the caller retries once.""" + get here: the index refuses the second, and the caller joins the one that won.""" now = datetime.now(timezone.utc) current = _unclosed(db, user) if current is not None: - if _is_live(current, now): + if is_live(current, now): return current end(current, EndReason.EXPIRED) db.flush() @@ -91,6 +98,12 @@ def start(db: Session, user: User, level: SessionLevel) -> DebugSession: return session +def stop(session: DebugSession) -> None: + """The user's Stop. Past its cap the session had already ended on its own, and a late Stop must + not rename that.""" + end(session, EndReason.STOPPED if is_live(session) else EndReason.EXPIRED) + + def end(session: DebugSession, reason: EndReason) -> None: """Close a session. An expired one is dated at its cap, not at whenever it was noticed.""" if session.ended_at is not None: @@ -111,7 +124,7 @@ def from_header(db: Session, user: User, value: str | None) -> DebugSession | No if ( session is None or session.user_id != user.id - or not _is_live(session, datetime.now(timezone.utc)) + or not is_live(session, datetime.now(timezone.utc)) ): return None return session @@ -127,11 +140,37 @@ def level_for(user: User, session: DebugSession | None) -> tuple[str, datetime | return session.level, session.until +def lock(db: Session, session: DebugSession) -> None: + """Re-read the session under a row lock, held until the caller commits. Everything that changes + what an issue is made of — the draft, the screenshots, the send itself — takes it first: a send + in flight sees none of them change under it, and each of them sees a send that committed.""" + db.refresh(session, with_for_update=True) + + def save_draft(session: DebugSession, draft: dict) -> None: - """Keep the composer's draft (the caller validated and commits). Shape: `title`, `body` (the - rendered markdown an export or an issue carries), `excluded` (log row ids taken out of the - evidence) and `form` (the composer's own fields, to reopen it as it was).""" - session.draft = draft + """Keep the composer's draft (the caller validated, locked and commits). Shape: `title`, `body` + (the rendered markdown an export or an issue carries), `excluded` (log row ids taken out of the + evidence) and `form` (the composer's own fields, to reopen it as it was). A delivery's progress + is not the composer's to replace, and is kept.""" + progress = (session.draft or {}).get("delivery") + session.draft = {**draft, "delivery": progress} if progress else draft + + +# --- a delivery's progress ----------------------------------------------------------------- +# Written by app/issues/gitea.py into the draft; read HERE, so the send's decision and what the +# composer shows are one reading of it. +def delivery(session: DebugSession) -> dict: + return dict((session.draft or {}).get("delivery") or {}) + + +def unfinished(session: DebugSession) -> list[str]: + """What a send that stopped half-way still owes its issue: the files not yet uploaded, and the + screenshots uploaded but not yet shown in its body (the edit that shows them failed).""" + state = delivery(session) + missing = list(state.get("pending") or []) + if not state.get("images_shown"): + missing += [n for n in state.get("links") or {} if n.startswith("screenshot-")] + return missing def serialize(session: DebugSession | None, screenshots: int = 0) -> dict | None: @@ -145,13 +184,18 @@ def serialize(session: DebugSession | None, screenshots: int = 0) -> dict | None "ended_at": session.ended_at.isoformat() if session.ended_at else None, "ended_reason": session.ended_reason, "issue_url": session.issue_url, - # A send that stopped half-way (app/issues/gitea.py): the files still to go. - "pending_files": ((session.draft or {}).get("delivery") or {}).get("pending") or [], + # A send that stopped half-way: what it still owes. + "pending_files": unfinished(session), "screenshots": screenshots, "draft": session.draft, } +def view(db: Session, session: DebugSession) -> dict: + """The session as the API serves it — one shape wherever it is served.""" + return serialize(session, screenshot_count(db, session)) or {} + + def screenshot_count(db: Session, session: DebugSession) -> int: return ( db.scalar( @@ -172,7 +216,7 @@ _SIGNATURES = ( def sniff(data: bytes) -> str | None: - """The image type `data` really is — PNG, JPEG or WebP — or None.""" + """The image type `data` really is — one of IMAGE_TYPES — or None.""" for magic, mime in _SIGNATURES: if data.startswith(magic): return mime @@ -181,6 +225,17 @@ def sniff(data: bytes) -> str | None: return None +def stored_screenshot_bytes(db: Session, user_id: int) -> int: + return ( + db.scalar( + select(func.coalesce(func.sum(DebugScreenshot.size), 0)) + .join(DebugSession, DebugSession.id == DebugScreenshot.session_id) + .where(DebugSession.user_id == user_id) + ) + or 0 + ) + + class ScreenshotRefused(Exception): """A screenshot that cannot be kept; `code` is the refusal's machine word.""" @@ -190,17 +245,23 @@ class ScreenshotRefused(Exception): def add_screenshot(db: Session, session: DebugSession, data: bytes) -> DebugScreenshot: - """Keep one screenshot. Flushes; the caller commits. Refused once the issue was sent (it can no - longer carry it), past the per-session count, or when it is not an image we can show.""" - if session.issue_url is not None: - raise ScreenshotRefused("already_sent") + """Keep one screenshot. Locks the session and flushes; the caller commits. Refused once the + issue was sent (it can no longer carry it), past the per-session count or the user's storage, + or when it is not an image we can show.""" if not data: raise ScreenshotRefused("empty") mime = sniff(data) if mime is None: raise ScreenshotRefused("not_an_image") + # Under the lock: a send in flight is waited for (and then refuses this), and two uploads at + # once cannot both pass the count. + lock(db, session) + if session.issue_url is not None: + raise ScreenshotRefused("already_sent") if screenshot_count(db, session) >= SCREENSHOTS_MAX: raise ScreenshotRefused("too_many") + if stored_screenshot_bytes(db, session.user_id) + len(data) > SCREENSHOT_USER_MAX_BYTES: + raise ScreenshotRefused("storage_full") shot = DebugScreenshot( session_id=session.id, created_at=datetime.now(timezone.utc), @@ -213,6 +274,16 @@ def add_screenshot(db: Session, session: DebugSession, data: bytes) -> DebugScre return shot +def remove_screenshot(db: Session, session: DebugSession, shot: DebugScreenshot) -> None: + """Delete one screenshot, under the lock. Refused once the issue was sent: it carries this one + already, and a delivery names the screenshots by their order. The caller commits.""" + lock(db, session) + if session.issue_url is not None: + raise ScreenshotRefused("already_sent") + db.delete(shot) + db.flush() + + def screenshots(db: Session, session: DebugSession) -> list[DebugScreenshot]: """The session's screenshots, oldest first, WITHOUT their bytes loaded.""" return list( diff --git a/backend/app/issues/bundle.py b/backend/app/issues/bundle.py index 07d02a7d..3484f3ef 100644 --- a/backend/app/issues/bundle.py +++ b/backend/app/issues/bundle.py @@ -23,8 +23,6 @@ from sqlalchemy.orm import Session from app import debugsession, logsink from app.models import DebugScreenshot, DebugSession -_EXT = {"image/png": "png", "image/jpeg": "jpg", "image/webp": "webp"} - @dataclass(frozen=True) class IssueFile: @@ -65,7 +63,8 @@ def files(db: Session, session: DebugSession) -> list[IssueFile]: .order_by(DebugScreenshot.id) ) for n, shot in enumerate(shots, start=1): - out.append(IssueFile(f"screenshot-{n}.{_EXT.get(shot.mime, 'bin')}", shot.data, shot.mime)) + ext = debugsession.IMAGE_TYPES[shot.mime] + out.append(IssueFile(f"screenshot-{n}.{ext}", shot.data, shot.mime)) return out @@ -73,5 +72,7 @@ def zipped(items: list[IssueFile]) -> bytes: buf = io.BytesIO() with zipfile.ZipFile(buf, "w", zipfile.ZIP_DEFLATED) as z: for item in items: - z.writestr(item.name, item.data) + # The images are compressed already: deflating them again costs time and wins nothing. + method = zipfile.ZIP_STORED if item.mime.startswith("image/") else zipfile.ZIP_DEFLATED + z.writestr(item.name, item.data, compress_type=method) return buf.getvalue() diff --git a/backend/app/issues/gitea.py b/backend/app/issues/gitea.py index ea0da7af..62de2c62 100644 --- a/backend/app/issues/gitea.py +++ b/backend/app/issues/gitea.py @@ -17,19 +17,20 @@ A private repository's attachment answers 404 to a browser not signed in to Gite composer's own preview shows our copies, never these links. Delivery keeps its progress in the session's draft (`delivery`: the issue number, the files still to -upload, the links of those uploaded), so a failure half-way keeps the issue that exists and a retry -sends only what is missing — never a second issue. Every step flushes; the caller commits, also -when a step fails, so what did happen is kept. +upload, the links of those uploaded, whether the body shows the screenshots yet), so a failure +half-way keeps the issue that exists and a retry does only what is missing — never a second issue. +Every step flushes; the caller holds the session's lock throughout (`debugsession.lock`) and commits, +also when a step fails, so what did happen is kept. `debugsession.unfinished` reads that progress. """ -from collections.abc import Iterator +from collections.abc import Callable, Iterator from contextlib import contextmanager -from typing import Literal +from typing import Any, Literal import httpx from sqlalchemy.orm import Session -from app import logs, sysconfig +from app import debugsession, logs, sysconfig from app.issues import bundle from app.models import DebugSession @@ -44,7 +45,8 @@ Reason = Literal[ "repo_not_found", # 404 on the repository "public_repo", # the repository is public — an issue would publish the reporter's log "issues_disabled", # the repository has its issue tracker switched off - "rejected", # any other 4xx (a file type the instance does not allow, …) + "rejected", # any other 4xx (a file type the instance does not allow, …), or an answer that is + # not the API's (a redirect — an http:// address behind https —, a page that is not JSON) ] @@ -91,7 +93,7 @@ def _call(client: httpx.Client, method: str, path: str, **kwargs) -> httpx.Respo extra=logs.event("issues.gitea.unreachable", path=path, error=type(exc).__name__), ) raise GiteaError("unreachable") from None - if r.status_code < 400: + if r.status_code < 300: return r # The status only: Gitea's message can quote back what was sent — the user's own title. log.warning( @@ -107,8 +109,25 @@ def _call(client: httpx.Client, method: str, path: str, **kwargs) -> httpx.Respo raise GiteaError("rejected") +def _json(r: httpx.Response, *keys: str) -> dict[str, Any]: + """The answer as the API's JSON, holding `keys` — anything else is refused, not a crash: a + crash after the issue was created would lose the record that it was.""" + try: + data = r.json() + # silent-ok: not JSON is one of the answers refused (and logged) just below. + except ValueError: + data = None + if not isinstance(data, dict) or any(k not in data for k in keys): + log.warning( + "the issue tracker answered something that is not its API", + extra=logs.event("issues.gitea.unexpected", status=r.status_code), + ) + raise GiteaError("rejected") + return data + + def _private_repo(client: httpx.Client, repo: str) -> dict: - data = _call(client, "GET", f"/repos/{repo}").json() + data = _json(_call(client, "GET", f"/repos/{repo}")) if not data.get("private"): raise GiteaError("public_repo") if not data.get("has_issues"): @@ -135,20 +154,30 @@ def _save(session: DebugSession, state: dict) -> None: session.draft = {**(session.draft or {}), "delivery": state} -def deliver(db: Session, session: DebugSession) -> str: - """Send the session's issue, or finish sending it; returns its URL. Flushes after every step.""" - state = dict((session.draft or {}).get("delivery") or {}) +def deliver( + db: Session, session: DebugSession, *, before_create: Callable[[], None] | None = None +) -> str: + """Send the session's issue, or finish sending it; returns its URL. Flushes after every step. + `before_create` runs right before a NEW issue is opened — after every check that could refuse + it — and may raise to stop it (the caller's pace on new issues).""" + state = debugsession.delivery(session) files = bundle.files(db, session) attachments = [f for f in files if f.name != "issue.md"] with _client(db) as (client, repo): _private_repo(client, repo) if "number" not in state: - created = _call( - client, - "POST", - f"/repos/{repo}/issues", - json={"title": bundle.title_of(session), "body": bundle.body_of(session)}, - ).json() + if before_create is not None: + before_create() + created = _json( + _call( + client, + "POST", + f"/repos/{repo}/issues", + json={"title": bundle.title_of(session), "body": bundle.body_of(session)}, + ), + "number", + "html_url", + ) state = { "number": created["number"], "pending": [f.name for f in attachments], @@ -165,13 +194,16 @@ def deliver(db: Session, session: DebugSession) -> str: for item in attachments: if item.name not in state["pending"]: continue - asset = _call( - client, - "POST", - f"/repos/{repo}/issues/{number}/assets", - params={"name": item.name}, - files={"attachment": (item.name, item.data, item.mime)}, - ).json() + asset = _json( + _call( + client, + "POST", + f"/repos/{repo}/issues/{number}/assets", + params={"name": item.name}, + files={"attachment": (item.name, item.data, item.mime)}, + ), + "browser_download_url", + ) state = { **state, "pending": [n for n in state["pending"] if n != item.name], @@ -187,8 +219,3 @@ def deliver(db: Session, session: DebugSession) -> str: _save(session, {**state, "images_shown": True}) db.flush() return session.issue_url or "" - - -def pending(session: DebugSession) -> list[str]: - """The files a half-finished delivery still has to send.""" - return list(((session.draft or {}).get("delivery") or {}).get("pending") or []) diff --git a/backend/app/logs.py b/backend/app/logs.py index 61c2dcc6..ce42b3dd 100644 --- a/backend/app/logs.py +++ b/backend/app/logs.py @@ -127,7 +127,11 @@ def set_user( from the request middleware, which runs OUTSIDE the session middleware. `debug_session` tags every line with the user's open debug session (E16). The caller has - already checked it is theirs and open; a session's level arrives here as `level`/`until`.""" + already checked it is theirs and open; a session's level arrives here as `level`/`until`. + + The root logger's floor is tracked per elevation: a request without the session's header (an + ``, a `