Issue #506: feat: Make logout summarization non-blocking

[dereck-symmetry]

Description of Change

Problem. /logout blocked on interaction summarization — an LLM round-trip — before returning. #506's note says it plainly: "kick off background task."

Solution. Move summarization into a FastAPI background task so /logout returns immediately, with the summarization wrapped in _safe_summarize so a failing or unavailable LLM cannot fail a logout.

async def logout(background_tasks: BackgroundTasks, username: str = Depends(get_current_user)):
    ...
    background_tasks.add_task(_safe_summarize, agent, task, username)

---

Review response

Both blocking items are fixed by the same change, which is why this ended up smaller than the review implied.

1 + 2. BackgroundTasks instead of a bare asyncio.create_task.

You were right about the discarded reference. The event loop keeps only a weak reference to a bare task, so it can be garbage-collected mid-flight — rare, load-dependent, and silent, which is the worst combination. Retaining it in a module-level set would have worked, but BackgroundTasks is the framework's own answer, and since this base had no existing pattern, it may as well establish the idiomatic one.

It also makes the tests deterministic, so both asyncio.sleep(0.1) barriers are gone — they were the only sleeps in the entire suite.

One correction: these tests use httpx AsyncClient over ASGITransport (test_core.py:14), not Starlette's TestClient, so the stated reason didn't transfer. I verified the conclusion holds anyway rather than assuming — ASGITransport awaits the full ASGI call, and Starlette runs background tasks inside it, so the task has finished by the time the request returns:

response: {'ok': True}
background task completed before client.post() returned?  True

Mutation-checked both ways, so the tests aren't passing by accident:

Mutation	Result
Don't schedule the task at all	both logout tests fail
Revert to asyncio.create_task, sleeps still removed	both logout tests fail

The second is the one that matters: it demonstrates the sleeps were doing real synchronization work, and that BackgroundTasks is what replaces them rather than just hiding them.

A trade-off I checked before adopting it. BackgroundTasks runs inside the request's ASGI call, so in principle it could hold the keep-alive connection and partly undo the latency win this PR exists for. Measured against a real uvicorn server with a 1.5s background task:

/bg  (BackgroundTasks):  response=3.4ms   next request on same connection=1.9ms
/ct  (create_task):      response=2.2ms   next request on same connection=1.5ms

No penalty. Recording it because it was a real reason not to adopt BackgroundTasks, and it turned out not to be.

3. Durability — documented and deferred, as you suggested.

You're right that this widens the window, and that the trade should be visible rather than discovered later: previously a successful logout implied the summary was saved; now it doesn't. There is no on_event("shutdown") or lifespan handler in this base, so an ECS task stop during a deploy or scale-in kills in-flight summarization silently.

Not solved here — a shutdown drain means deciding how long to wait, which is a design question, and #1118 already owns best-effort summary capture. Now carried as Refs #1118 rather than left implicit.

4. TODO(#986) restored — carried into _safe_summarize alongside the prompt it annotates.

Small correction for the record: it wasn't #986's only in-code anchor. main has two — the load_profile prompt at core.py:266 and this one. This branch dropped one of the two.

Also: _safe_summarize's docstring said it "swallows errors" while actually logging a stack trace via logger.exception. That behavior is the point — an absent summary stays diagnosable — so the docstring now says what it does.

Related Issues

Closes #506
Refs #1131
Refs #1118
Refs #986

Type of Change

• Performance improvement
• Bug fix (non-breaking change which fixes an issue)

Project Area(s) Affected

• bases/
• test/ or e2e/

---

Checklist

• commit message follows commit guidelines (see commitlint.config.mjs)
• tests are included (unit and/or integration tests)
• code passes linting checks (uv run ruff check)
• code passes formatting checks (uv run ruff format)
• code passes type checking (uv run ty check)

Testing

• Automated tests added/updated

Check	Result
uv run ruff check	pass
uv run ruff format --check	pass
uv run ty check --error-on-warning	pass
uv run pytest test	642 passed, 49 skipped
uv run pytest test/bases/lif/advisor_restapi/	18 passed, no sleeps
cspell-cli@9.0.1 lint (changed files)	pass

Plus the two mutations and the keep-alive measurement above.

Additional Notes

• main merged in, not rebased (e140f74), with the review round appended as fa40f52. No force-push, so reviewer state is intact. The merge conflict was a single line — this branch added import asyncio to the test file, main didn't — and it dissolved on its own, since that import existed only for the two sleeps.
• Behavior change worth stating plainly: a 200 from /logout no longer means the interaction summary was saved. It means the summarization was scheduled. See Refs #1118.

[bjagg]

Overview

Makes /logout return immediately by moving interaction summarization into a background task, fixing #506 ("kick off background task"). Small and focused, and the error handling is right: _safe_summarize catches broadly but uses logger.exception rather than a bare pass, so failures are still visible. test_logout_succeeds_even_when_summarization_fails is exactly the test I'd want here — it proves the user-facing path is genuinely decoupled from LLM availability.

Two things I'd like changed before merge, and a couple of notes.

1. The task reference is discarded

asyncio.create_task(_safe_summarize(agent, task, username))

The return value isn't kept. The event loop holds only a weak reference to a running task, so this can be garbage-collected mid-execution — the CPython docs call this out directly and recommend saving a reference. The failure mode is nasty precisely because it's rare and load-dependent: summaries silently vanish some fraction of the time, with nothing in the logs.

Minimal fix:

_background_tasks: set[asyncio.Task] = set()

task_ref = asyncio.create_task(_safe_summarize(agent, task, username))
_background_tasks.add(task_ref)
task_ref.add_done_callback(_background_tasks.discard)

2. FastAPI's BackgroundTasks is the better fit here

FastAPI has a first-class mechanism for "do this after the response is sent":

async def logout(background_tasks: BackgroundTasks, username: str = Depends(get_current_user)):
    ...
    background_tasks.add_task(_safe_summarize, agent, task, username)

It solves the reference problem for you, and — the part I care more about — it makes the tests deterministic. TestClient runs background tasks to completion before returning, so the two await asyncio.sleep(0.1) calls disappear.

Those sleeps are the second thing I'd change. A fixed 100ms as a synchronization barrier is timing-dependent and will flake under CI load; grep says these would be the only such sleeps in the whole test suite. If BackgroundTasks isn't wanted for some reason, the alternative is to capture the task and await it in the test rather than sleeping.

Worth noting the repo doesn't use BackgroundTasks anywhere yet, so either way this establishes a pattern — which is an argument for establishing the framework-idiomatic one.

3. Nothing survives shutdown — and this weakens #1118

bases/lif/advisor_restapi/core.py has no on_event("shutdown") or lifespan handler. When an ECS task stops (a deploy, a scale-in, a health-check failure), any in-flight summarization is killed and the interaction summary is lost with no trace beyond an absent log line.

That matters because #1118 already tracks interaction-summary capture being best-effort and logout-only. This change makes the window wider: previously the summary completed before the response returned, so a user who logged out successfully had their summary saved. Now a successful logout no longer implies that.

Not asking you to solve durability here — that's #1118's job. But it's worth a line in the PR description or a Refs #1118, because this trades a latency win for a durability loss and that trade should be visible rather than discovered later.

4. The TODO(#986) marker was dropped

The prompt string moved into _safe_summarize, but the comment went with it:

# TODO(#986): move this hard-coded query prompt to env/config

#986 is still open. That comment was its only in-code anchor — worth carrying it over to the new location.

Verdict

Approve once items 1 and 2 are addressed — the unretained task is a real (if intermittent) correctness issue, and the sleeps will cost someone a flaky-CI investigation. Items 3 and 4 are notes, not blockers.

The core change is right and the failure-path test is a good addition.

[dereck-symmetry]

Addressed in fa40f52, with main merged at e140f74 rather than rebased — nothing force-pushed. CI green, conflict gone. Detail is in the PR description; the short version:

Items 1 and 2 turned out to be one change. Switched to background_tasks.add_task(...). You were right about the weak reference — the silent, load-dependent failure mode is the worst kind. Retaining the task in a module-level set would have worked, but since this base had no existing pattern, the framework-idiomatic one seemed the better thing to establish. That also made the tests deterministic, so both sleep(0.1) barriers are gone.

One correction: these tests use httpx AsyncClient over ASGITransport, not Starlette's TestClient, so the reason you gave didn't transfer. I checked the conclusion holds anyway rather than assuming — ASGITransport awaits the full ASGI call and Starlette runs background tasks inside it, so the task is finished before the request returns. Right answer, different mechanism.

Mutation-checked, because "the tests still pass" proves nothing here: removing the scheduling fails both logout tests, and reverting to asyncio.create_task with the sleeps already gone also fails both. That second one is the useful one — it shows the sleeps were doing real synchronization work rather than being decorative, and that BackgroundTasks genuinely replaces them.

I nearly didn't take your suggestion. BackgroundTasks runs inside the request's ASGI call, so I thought it might hold the keep-alive connection and partly undo the latency win this PR exists for. Measured it against a real uvicorn server with a 1.5s background task: next request on the same connection came back in 1.9ms, versus 1.5ms for create_task. No penalty — so the objection was unfounded and your suggestion stands on its own.

Item 3, documented and deferred as you proposed. The PR now carries Refs #1118 and says plainly that a 200 from /logout no longer means the summary was saved — only that it was scheduled. Confirmed there's no on_event("shutdown") or lifespan handler in this base, so an ECS task stop does lose in-flight work. Solving that means deciding a drain timeout, which is a design question and #1118's to own.

Item 4 restored, with one correction for the record: it wasn't #986's only in-code anchor — main has two, the load_profile prompt at core.py:266 and this one. This branch dropped one of the two.

Also fixed _safe_summarize's docstring, which claimed it "swallows errors" while actually logging a stack trace. The logging is the point you praised, so the docstring now says so.

The conflict resolved itself, incidentally: it was the import asyncio line the test file added, which existed only for the sleeps.

No rush on the re-review.