Repository navigation
Fix stale HTTP connection events and test port ownership - #483
Merged
Merged
Conversation
The HTTP transport rebuilt connection identity from recycled file descriptors in saved poll events and deferred callbacks, and checked a flush target before waiting for the socket lock. Once the operating system reused a descriptor, a stale event could close or flush the replacement connection. Browsers saw this as connection resets and as HTTP 200 responses followed by a truncated body. The fork at valthon/zap 2bfc918 keeps the registered generation for poll snapshots and deferred callbacks and revalidates it under the connection lock for callbacks, watcher changes, flush and close. It keeps issue #380's protection for descriptors outside facil.io, so a later re-pin has to carry both fixes. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01HTAHrkfe1L2hfpLeP1CQAS
The browser fixtures released an ephemeral port before the server child bound it, so another worker could take the port and a foreign listener could satisfy the health check for a server that never started. Fixtures now hold a loopback reservation across the spawn and bind, and readiness requires the spawned PID's own bind evidence as well as a healthy response. Logs are read incrementally from the launch offset, so a restart ignores earlier output. A bind failure or exhausted readiness fails setup and releases the child, directory and port, including in the unsubscribe fixture. A failed test now reports the server's PID, exit status and log, also under xdist, and a server that exits before fixture cleanup fails the test instead of passing unnoticed. test_server_ownership covers the reservation and readiness paths. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_01HTAHrkfe1L2hfpLeP1CQAS
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Transport-maintainer review is needed to validate the pinned fork’s lock ordering, descriptor reuse, and cross-platform synchronization changes, which are outside this checkout and cannot be verified by the available local check.
No open issues 🎉
What changed in this PR
This PR updates ZigBase’s HTTP transport dependency to address stale connection events and strengthens test-server startup handling to reduce port races and misleading readiness checks.
Changes:
- Pins the zap fork containing connection-generation safeguards while retaining the descriptor protection associated with #380.
- Adds port reservations, process-aware readiness checks, incremental startup-log reads, and failure diagnostics to the shared test harness.
- Migrates test launchers to reserved ports and adds regression coverage for ownership, startup failures, and cleanup.
| File | Description |
|---|---|
tests/admin/test_thumbnails.py |
Reserves ports for thumbnail server tests. |
tests/admin/test_superuser_cli.py |
Reserves the server port during CLI authentication tests. |
tests/admin/test_static_files.py |
Uses shared port reservations across static-file tests. |
tests/admin/test_state.py |
Reserves ports for state and custom-response tests. |
tests/admin/test_server_ownership.py |
Adds readiness, port ownership, restart-log, and cleanup regression tests. |
tests/admin/test_scheduler.py |
Reserves the scheduler example’s server port. |
tests/admin/test_resumable_uploads.py |
Reserves ports and isolates readiness logs across restarts. |
tests/admin/test_rest_retry_integration.py |
Reserves the retry integration fixture’s port. |
tests/admin/test_rest_idempotency.py |
Reserves ports for multi-process and database coordination tests. |
tests/admin/test_query_workbench_postgres.py |
Reserves ports for live PostgreSQL workbench tests. |
tests/admin/test_memory_jobs.py |
Reserves the memory-job fixture’s server port. |
tests/admin/test_mail_unsubscribe.py |
Adds checked readiness, captured logs, and scoped resource cleanup. |
tests/admin/test_import.py |
Holds a port reservation through import-server startup. |
tests/admin/test_import_external_auths.py |
Holds a port reservation through external-auth import startup. |
tests/admin/test_file_range.py |
Reserves the file-serving fixture’s port. |
tests/admin/test_durable_uploads.py |
Reserves primary and competing ports and tracks restart-log offsets. |
tests/admin/test_durable_realtime.py |
Reserves the durable realtime fixture’s port. |
tests/admin/test_custom_route.py |
Reserves the custom-route example’s server port. |
tests/admin/test_agent_contracts.py |
Reserves ports for logged-server fixtures. |
tests/admin/conftest.py |
Adds shared reservations, process-aware readiness, cleanup, and diagnostics. |
changelog.d/http-connection-generation.md |
Documents the connection-reset and truncated-response fix. |
build.zig.zon |
Updates the zap pin and documents the required transport safeguards. |
Review effort: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The HTTP transport rebuilt connection identity from recycled file descriptors in saved poll events and deferred callbacks, and checked a flush target before waiting for the socket lock. Once a descriptor was reused, a stale event could close or flush the replacement connection, which browsers saw as connection resets and as HTTP 200 responses with a truncated body.
The zap fork at
2bfc918ff1e07ced69fb5535c19b56251c9fcea0keeps the registered generation and revalidates it under the connection lock for callbacks, watcher changes, flush and close. It also keeps the protection for descriptors outside facil.io from #380.Separately, the browser fixtures released an ephemeral port before the server child bound it, so a foreign listener could satisfy the health check. Fixtures now hold a reservation through startup and require the spawned PID's bind evidence and a healthy response. Logs are read incrementally from the launch offset. A failed start releases the child, directory and port. A failed test reports the server's PID, exit status and log.
The series is two commits: the dependency pin, then the test harness. Both were previously mixed into #481 and #482, and are removed from those PRs. The prior heads are preserved on
codex-does-too-much-481andcodex-does-too-much-482.Validation on Zig 0.16.0, on
mainat7e212f30:zig build test --summary allreturned 0 at both commits.tests/adminwith-n 4: 380 passed, 154 skipped, one small test-only change before this head.test_server_ownership.py(14 passed) andtest_resumable_uploads.pyagainst the resumable fixture (15 passed) were re-run at this head.check-gating, allocator contracts, disabled agent CLI, error-code ledger, doctor ledger and changelog assembly passed.The zap fork's own source was not reviewed as part of this PR. Its hosted Ubuntu and macOS checks passed when it was built.
🤖 Generated with Claude Code
https://claude.ai/code/session_01HTAHrkfe1L2hfpLeP1CQAS