fix(diagnostics): deep-review round 2 — a send that claims, not one that blocks
The send no longer holds the session's row lock while it talks to the tracker: it claims the session, gives the lock back, and checkpoints each step. A Stop is never held behind a send, while a draft, a screenshot or a second send are refused at once instead of queueing. The claim ages out, so a request that died mid-send does not strand the session. What the composer says is what happened: an attachment that went is no longer named as one that did not (the missing body edit is its own line), and the issue's screenshot count comes from the server when the list cannot be read, so a failed read no longer blocks saving the draft. The zip is fetched as a request, so its pace is a message and not a saved error page. Screenshots of a finished report can be removed again; only one still being sent is held. A recording's log elevation can no longer outlive it. The step prompt keeps the keyboard (the page's keys stay quiet) and traps Tab itself. An image pasted into a dialog opened over the composer stays that dialog's.
This commit is contained in:
@@ -147,9 +147,11 @@ def save_draft(
|
||||
session = owned_or_404(db, DebugSession, sid, user, what="debug_session")
|
||||
if len(json.dumps(body.form, ensure_ascii=False).encode()) > FORM_MAX_BYTES:
|
||||
raise HTTPException(status_code=413, detail={"code": "too_large"})
|
||||
# Under the lock: a save that arrives while the issue is being sent waits for the send, and then
|
||||
# sees it — the draft of a sent issue cannot change after it.
|
||||
# Under the lock, and refused while a send is under way: the issue is being built from the draft
|
||||
# as it was saved, and this would change it under the send.
|
||||
debugsession.lock(db, session)
|
||||
if debugsession.sending(session):
|
||||
raise refused(409, "issue_sending")
|
||||
if session.issue_url is not None:
|
||||
# Sent: the issue is on the tracker now, and a draft edited after it would say otherwise.
|
||||
raise refused(409, "issue_already_sent")
|
||||
@@ -181,9 +183,10 @@ def export_session(
|
||||
|
||||
|
||||
# --- the issue tracker (S4) -----------------------------------------------------------------
|
||||
# Issues a user may OPEN per hour — finishing a half-sent one, or an attempt refused before the issue
|
||||
# was opened, is not counted. The composer sends one at a time; this is room for a bad afternoon, not
|
||||
# a pace anything legitimate keeps up.
|
||||
# Issues a user may open per hour — counted once the tracker is ASKED to open one (an answer that
|
||||
# goes missing may still have created it, so an attempt cannot be given back); finishing a half-sent
|
||||
# one, or an attempt refused before the tracker was asked, is not counted. The composer sends one at
|
||||
# a time; this is room for a bad afternoon, not a pace anything legitimate keeps up.
|
||||
_new_issues = RateLimiter(max_events=5, window_seconds=3600)
|
||||
|
||||
|
||||
@@ -198,10 +201,12 @@ def send_issue(
|
||||
"""Send the session's issue to the tracker — from the SAVED draft, as the export is built — or
|
||||
finish a send that stopped half-way (the issue exists; only what is missing is sent)."""
|
||||
session = owned_or_404(db, DebugSession, sid, user, what="debug_session")
|
||||
# Held to the end: a second send (another tab, a retry after a timeout) waits for this one and
|
||||
# then finds the issue it opened, instead of opening another; an autosave or a screenshot waits
|
||||
# too, and then finds the issue sent.
|
||||
# The session is CLAIMED for the send rather than locked through it: the tracker is slow (a
|
||||
# dozen calls, 20 s each), and a row lock held across them would queue this user's Stop, their
|
||||
# autosave and their screenshots behind it. They see the claim instead and are refused at once.
|
||||
debugsession.lock(db, session)
|
||||
if debugsession.sending(session):
|
||||
raise refused(409, "issue_sending")
|
||||
new = session.issue_url is None
|
||||
if not new and not debugsession.unfinished(session):
|
||||
return {"ok": True, "session": _view(db, session)}
|
||||
@@ -211,30 +216,41 @@ def send_issue(
|
||||
raise refused(422, "issue_title_required")
|
||||
if _new_issues.exhausted(key):
|
||||
raise _issues_rate_limited()
|
||||
debugsession.claim_send(session)
|
||||
db.commit() # the claim is what everything else sees; the lock ends here
|
||||
|
||||
def count_new_issue() -> None:
|
||||
if not _new_issues.allow(key):
|
||||
raise _issues_rate_limited()
|
||||
|
||||
try:
|
||||
gitea.deliver(db, session, before_create=count_new_issue)
|
||||
gitea.deliver(db, session, checkpoint=db.commit, before_create=count_new_issue)
|
||||
except gitea.GiteaError as exc:
|
||||
# Keep what did happen: an issue created before an upload failed stays linked, and the
|
||||
# retry does only what is still missing.
|
||||
_record_created(db, user, session, new)
|
||||
db.commit()
|
||||
raise issue_refused(exc) from None
|
||||
except HTTPException:
|
||||
raise # the pace, refused before anything was opened
|
||||
except Exception:
|
||||
# Anything else after the issue was opened must not lose the record that it was: a retry
|
||||
# would open a second one.
|
||||
raise # the pace, refused before the tracker was asked
|
||||
finally:
|
||||
_end_send(db, user, session, new)
|
||||
return {"ok": True, "session": _view(db, session)}
|
||||
|
||||
|
||||
def _end_send(db: Session, user: User, session: DebugSession, new: bool) -> None:
|
||||
"""Give the claim back, and record an issue this send opened — whatever else happened. A
|
||||
failure here must not replace the failure that brought us: a dead transaction (the database
|
||||
went away mid-send) is rolled back, and the claim then simply ages out."""
|
||||
try:
|
||||
debugsession.lock(db, session)
|
||||
debugsession.release_send(session)
|
||||
_record_created(db, user, session, new)
|
||||
db.commit()
|
||||
raise
|
||||
_record_created(db, user, session, new)
|
||||
db.commit()
|
||||
return {"ok": True, "session": _view(db, session)}
|
||||
except Exception: # noqa: BLE001 — whatever went wrong here, the send's own failure is the news
|
||||
db.rollback()
|
||||
log.warning(
|
||||
"a send could not give its claim back",
|
||||
extra=logs.event("issues.claim.stuck", session_id=session.id),
|
||||
)
|
||||
|
||||
|
||||
def _record_created(db: Session, user: User, session: DebugSession, new: bool) -> None:
|
||||
@@ -315,6 +331,8 @@ def _screenshot_refused(code: str) -> HTTPException:
|
||||
return refused(409, "screenshot_already_sent")
|
||||
if code == "too_many":
|
||||
return refused(422, "screenshot_too_many", limit=debugsession.SCREENSHOTS_MAX)
|
||||
if code == "sending":
|
||||
return refused(409, "issue_sending")
|
||||
if code == "storage_full":
|
||||
mb = debugsession.SCREENSHOT_USER_MAX_BYTES // (1024 * 1024)
|
||||
return refused(422, "screenshot_storage_full", limit=mb)
|
||||
|
||||
Reference in New Issue
Block a user