Skip to content

feat(httpserver): add request lifecycle signals - #158

Merged
Oaklight merged 4 commits into
masterfrom
worktree-feat+lifecycle-signals
Sep 15, 2026
Merged

Oaklight merged 4 commits into
masterfrom
worktree-feat+lifecycle-signals

Conversation

@Oaklight

Copy link
Copy Markdown
Owner

Summary

  • Add on_response_started, on_response_completed, and on_client_disconnect signal decorators to App for fine-grained request lifecycle observability
  • on_response_started fires before response write — useful for TTFB metrics
  • on_response_completed fires after successful response delivery — useful for total transfer time
  • on_client_disconnect fires on broken pipe / connection reset — useful for cleanup and metrics
  • All signals support sync/async handlers, suppress exceptions, and work with both regular and streaming responses
  • StreamingResponse._write() now re-raises disconnect errors so _handle_connection can fire on_client_disconnect
  • 14 new tests covering all three signals, both response types, sync/async handlers, exception suppression, and ordering

Closes #136

Test plan

  • make test-httpserver — 109 passed, 0 failed (95 existing + 14 new)
  • Pre-commit hooks pass (ruff, ruff-format, ty, complexipy)
  • No regressions in existing streaming, middleware, lifespan, or cookie tests

@clementine-oaklight clementine-oaklight Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean addition — the three-signal design covers the full request lifecycle well, the try/except/else nesting in _handle_connection is correct, and the 14 tests are thorough (mock writers, streaming variants, ordering, exception suppression, timing verification). CI green 3.10–3.13.

1. on_response_started doesn't receive response

The signal fires after _dispatch() returns and before _write() — the response object exists at this point. For TTFB metrics you often want the status code / content-type alongside timing:

@app.on_response_started
async def ttfb(request):
    # Can't log response.status_code here
    request.state.response_start = time.monotonic()

You can work around it via on_response_completed, but then the timestamp is post-transfer, not pre-write. Passing (request, response) to on_response_started would close the gap — same signature as on_response_completed and no cost since the object already exists.

2. StreamingResponse._write() re-raise changes the method's contract

Previously _write() silently caught disconnect errors; now it re-raises. This is the right design (centralizes disconnect handling in _handle_connection), but it changes the contract of a method that has a clear "write and handle errors internally" pattern. Worth a brief docstring note on _write() that callers must handle BrokenPipeError / ConnectionResetError / ConnectionAbortedError, since _write is used in only one place but defines the boundary between response serialization and connection lifecycle.

3. on_client_disconnect doesn't fire for dispatch-phase disconnects

If the client disconnects while _dispatch() is running (e.g., during a slow handler that reads from the request body), the resulting exception propagates to the outer except Exception — on_client_disconnect never fires. The signal only covers write-phase disconnects. This is probably the correct scope (naming says "client disconnect," not "connection lost"), but documenting it in the docstring ("fired when the client disconnects during response delivery") would set expectations.

4. Incidental manifest.json diff

The jsonschema module's content_hash and last_updated changed in this PR's manifest. Looks like a rebase artifact from master. Not blocking but adds noise — could be cleaned up with a regenerate on the final merge commit.

Non-blocking

  • Hook iteration is sequential — three for hook in handlers loops run one at a time. Fine for the common case (1-2 lightweight hooks), but a slow sync hook in on_response_started delays the actual response write. A note in the docstring ("handlers run sequentially; keep them fast") would help.
  • on_response_completed fires before the finally block's writer.close() — so the response is "completed" from the server's perspective, but the TCP connection isn't fully torn down yet. Correct behavior, just noting it's intentional.

@Oaklight

Copy link
Copy Markdown
Owner Author

Addressed all points in 992e485:

  1. on_response_started now receives (request, response) — same signature as on_response_completed. Tests updated.
  2. StreamingResponse._write() docstring — documents that disconnect errors propagate to the caller, with generator cleanup and background still running via finally.
  3. on_client_disconnect scoped to response delivery — docstring now says "during response delivery" and explicitly notes it doesn't fire for dispatch-phase disconnects.
  4. manifest.json regenerated — clean diff, no rebase artifacts.
  5. Sequential execution note — added "handlers run sequentially; keep them fast" to on_response_started and on_response_completed docstrings.

@elena-oaklight elena-oaklight Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — feat(httpserver): add request lifecycle signals

Clean addition. The three-signal model (on_response_started / on_response_completed / on_client_disconnect) maps well to the ASGI lifecycle, the StreamingResponse._write re-raise is the right structural fix, and the test suite is thorough (14 tests covering sync/async, exception suppression, ordering, streaming, mid-stream disconnect, timing, background tasks). LGTM with two items to consider:


1. on_response_started could receive response — signature lock-in concern

The response object is already materialized when the signal fires (after _dispatch, before _write). Right now the handler gets (request) only, but ASGI's equivalent http.response.start includes status and headers. Users wanting TTFB-by-status-code metrics would need both on_response_started and on_response_completed to correlate the start timestamp with the status code.

Since this is a public API and changing (request) → (request, response) later is a breaking change, worth considering the two-arg signature now:

for hook in self._on_response_started_handlers:
    try:
        await _invoke(hook, request, response)  # response already available
    except Exception:
        logger.warning("on_response_started hook failed", exc_info=True)

Non-blocking — the current signature is coherent and documented; this is a forward-compatibility consideration.

2. BrokenPipeError during _dispatch now escalates from DEBUG → ERROR

The old structure caught (BrokenPipeError, ConnectionResetError, ConnectionAbortedError) around the combined _dispatch + _write block. The refactored structure only catches them around _write, so if _dispatch raises one of these (e.g. a before_request hook doing external IO), it falls through to the outer except Exception → logger.exception(...) — a full ERROR traceback instead of the quiet logger.debug("Connection reset by %s ...").

In practice this is unlikely since _dispatch doesn't touch the writer, but it is a behavioral change. Easy fix if desired — wrap _dispatch in its own guard:

try:
    response = await self._dispatch(request)
except (BrokenPipeError, ConnectionResetError, ConnectionAbortedError):
    logger.debug("Connection reset by %s during dispatch", client_addr)
    return

What looks good:

  • StreamingResponse._write re-raise is clean — finally still runs aclose(), and the only caller (_handle_connection) now catches properly
  • _invoke with asyncio.to_thread for sync handlers is correct — await preserves ordering guarantees
  • Exception suppression with per-hook try/except Exception + logger.warning is the right pattern (one failing hook doesn't block others or crash the response)
  • Test coverage is excellent — the _MidStreamDisconnectWriter (succeeds on headers, fails on body) is a nice touch for verifying both on_client_disconnect and background fire on partial streaming failures
  • Version bump 0.4.0 → 0.5.0 is appropriate for new public API surface

@milo-oaklight milo-oaklight Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean addition — the three-signal model maps well to the request lifecycle, the StreamingResponse._write re-raise fixes a genuine inconsistency (streaming disconnects were silently swallowed while non-streaming ones propagated), and the 14 tests are thorough. CI green across the board. LGTM.

1. Extract a _fire_signal helper

The three signal-firing loops in _handle_connection are nearly identical — iterate handlers, _invoke, try/except Exception, logger.warning. A small helper would cut ~15 lines and keep the semantics in one place:

async def _fire_signal(self, name: str, handlers: list, *args) -> None:
    for hook in handlers:
        try:
            await _invoke(hook, *args)
        except Exception:
            logger.warning("%s hook failed", name, exc_info=True)

Then the calling code becomes three one-liners:

await self._fire_signal("on_response_started", self._on_response_started_handlers, request)
await self._fire_signal("on_response_completed", self._on_response_completed_handlers, request, response)
await self._fire_signal("on_client_disconnect", self._on_client_disconnect_handlers, request)

2. +1 on passing response to on_response_started

Both Clementine and Elena flagged this — the response object exists, the signature would become symmetric with on_response_completed, and it avoids lock-in on a public API. Agree this is worth doing now.

3. Document signal ordering relative to middleware

Users will want to know where lifecycle signals sit in the existing chain. A one-line note in the class docstring would preempt the question:

before_request → route handler → after_request → on_response_started → write → on_response_completed
                                                                          ↘ on_client_disconnect

Non-blocking

  • Elena's dispatch-phase disconnect point is good — _dispatch raising BrokenPipeError now falls through to the generic except Exception (ERROR-level traceback). Wrapping _dispatch in its own (BrokenPipeError, ConnectionResetError, ConnectionAbortedError) guard or adding it to the existing catch would preserve the quiet DEBUG log.
  • Manifest noise — the jsonschema content_hash/last_updated diff is a rebase artifact. Regenerating on merge would clean it up.

@Oaklight

Copy link
Copy Markdown
Owner Author

Addressed Elena's feedback in 7ac3101:

  1. on_response_started signature — already fixed in the previous commit (992e485), now receives (request, response).
  2. BrokenPipeError during _dispatch escalation — added a guard around _dispatch() that catches BrokenPipeError/ConnectionResetError/ConnectionAbortedError and logs at DEBUG level, matching prior behavior. Without this, a disconnect during dispatch would fall through to except Exception → logger.exception() (ERROR traceback).

@Oaklight

Copy link
Copy Markdown
Owner Author

Addressed Milo's remaining feedback in 8c44f93:

  1. Extracted _fire_signal helper — the three signal-firing loops in _handle_connection now call self._fire_signal(name, handlers, *args), cutting ~15 lines of repeated try/except/warning logic.
  2. Added lifecycle diagram to App class docstring — shows signal ordering relative to middleware:
    before_request → route handler → after_request
      → on_response_started → write → on_response_completed
                                ↘ on_client_disconnect
    

All 109 tests pass, pre-commit clean.

Add on_response_started, on_response_completed, and on_client_disconnect
signal decorators for fine-grained request lifecycle observability.

- on_response_started: fires before response write (TTFB metrics)
- on_response_completed: fires after successful response delivery
- on_client_disconnect: fires on broken pipe / connection reset

All signals support sync/async handlers, suppress exceptions, and work
with both regular and streaming responses. StreamingResponse._write()
now re-raises disconnect errors so _handle_connection can fire the
on_client_disconnect signal.

Closes #136
- on_response_started now receives (request, response) so handlers
  can inspect status code / content-type alongside TTFB timing
- Document StreamingResponse._write() re-raise contract in docstring
- Scope on_client_disconnect docstring to response-delivery phase
- Add "handlers run sequentially" note to signal docstrings
- Regenerate manifest.json to remove rebase noise
Wrap _dispatch() in its own BrokenPipeError/ConnectionResetError guard
so a disconnect during dispatch logs at DEBUG level (matching prior
behavior) instead of falling through to the outer except Exception
which logs a full ERROR traceback.
…iagram

- Add _fire_signal() method to deduplicate the three signal-firing loops
  in _handle_connection (addresses Milo's review feedback)
- Add request lifecycle diagram to App class docstring showing signal
  ordering relative to middleware
@Oaklight
Oaklight force-pushed the worktree-feat+lifecycle-signals branch from 8c44f93 to 72a90d2 Compare September 15, 2026 01:27
@Oaklight
Oaklight merged commit 2fbbb83 into master Sep 15, 2026
6 checks passed
@Oaklight
Oaklight deleted the worktree-feat+lifecycle-signals branch September 15, 2026 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(httpserver): add fine-grained request lifecycle signals

1 participant