Session-1 resolution section records the concurrent-drainer fix shape, the bug-validated regression test (fails with the old order reinstated), the two P13 boundary/discard tests, and the deferred accept-loop harness item. Verification counts updated (98 lib / 141 --all-features).
This commit is contained in:
@@ -26,6 +26,8 @@ tool: two subagent reviews (code review + packaging/docs vs alkcall 0.4.1) + man
|
||||
verification of every finding against source + empirical deadlock reproduction
|
||||
reviewer: prepublish review for v0.1.0 (subagent-assisted)
|
||||
base_commit: 918af40 (review #002 resolved)
|
||||
resolutions:
|
||||
- "P1 + P13 resolved 2026-09-05 (87c52e5) — see the Session-1 Resolution section"
|
||||
---
|
||||
|
||||
# Code Review #003 — Prepublish Review (v0.1.0)
|
||||
@@ -462,7 +464,7 @@ theirs; don't replicate that nit.
|
||||
|
||||
| ID | Finding | Fix | Effort | Risk | Status |
|
||||
|----|---------|-----|--------|------|--------|
|
||||
| P1 | pump_session deadlock ≥63 chunks | concurrent drainer + regression test | small | medium (core pump) | open |
|
||||
| P1 | pump_session deadlock ≥63 chunks | concurrent drainer + regression test | small | medium (core pump) | ✅ resolved (`87c52e5`) |
|
||||
| P2 | AGENTS.md ships; dead exclude entries | fix `exclude` | trivial | none | open |
|
||||
| P3 | stale AGENTS.md + architecture README | refresh phase/ADR/alkcall text | trivial | none | open |
|
||||
| P4 | no README + `readme` field | write README (after P5/P6/P12) | medium | none | open |
|
||||
@@ -474,7 +476,7 @@ theirs; don't replicate that nit.
|
||||
| P10 | thread-spawn `.expect()` × 3 | map to `TtyError` | trivial | none | open |
|
||||
| P11 | recv_stdout sentinel contract | terminate stream on sentinel (or fix doc) | small | low (semver-visible) | open |
|
||||
| P12 | no crate-root re-exports | `pub use` primary types | trivial | none | open |
|
||||
| P13 | test gaps | backpressure (>64 chunks) + boundary tests | small | none | open |
|
||||
| P13 | test gaps | backpressure (>64 chunks) + boundary tests | small | none | ✅ resolved (`87c52e5`; accept-loop harness deferred) |
|
||||
| P14 | `Result<(), String>` on register_openable | closed — alkcall parity | none | none | ✅ closed |
|
||||
| P15 | minor polish batch | one batched commit | small | none | open |
|
||||
| P16 | no CHANGELOG.md | start changelog with [0.1.0] | small | none | open |
|
||||
@@ -494,6 +496,42 @@ Natural break points, cheapest-dependency order first:
|
||||
5. **P2 + P3 + P15** — packaging/docs closeout; then final
|
||||
`cargo publish --dry-run` and tag.
|
||||
|
||||
## Session-1 Resolution (P1 + P13, 2026-09-05)
|
||||
|
||||
P1 and P13 were resolved together (the regression test is part of the
|
||||
fix). Commit: `87c52e5`.
|
||||
|
||||
- **P1 fix**: `pump_session` now spawns the drainer (`drain_chunks`, a
|
||||
new function owning `writer_rx` + `client_write`) *before* the
|
||||
pumps+exit join; after the join, `send_exit_chunk` enqueues as
|
||||
before, then `drop(writer_tx)` lets the drainer observe channel close
|
||||
and finish. The single FIFO preserves exit-chunk-is-last (ADR-005);
|
||||
`W`'s existing `'static + Send + Unpin` bounds permitted moving the
|
||||
writer into the task. The doc comment on `pump_session` was updated
|
||||
to describe the concurrent-drainer shape and why it is load-bearing.
|
||||
- **P1 regression test** (`adapter::tests::backpressure_more_chunks_than_channel_capacity`):
|
||||
drives 80 chunks (> the 64-slot channel) through a real
|
||||
`drive_session` via the `TestBackend` harness, asserts in-order
|
||||
delivery of all 80 + sentinel + exit, with a 10 s per-read timeout so
|
||||
a regression fails the test instead of hanging it (the test body was
|
||||
validated against the bug: with the old order reinstated, it fails
|
||||
with "chunk read must not stall (P1 regression: deadlock): Elapsed" —
|
||||
the producer is spawned so the test's own sends cannot park first).
|
||||
- **P13 boundary test** (`wire::tests::chunk_at_exact_max_len_boundary`):
|
||||
a payload of exactly `MAX_CHUNK_LEN` round-trips (the limit is
|
||||
inclusive); the existing `chunk_too_large` covers over-limit.
|
||||
- **P13 discard test** (`session::tests::read_pump_ignores_client_to_server_stream_types_from_server`):
|
||||
stream_type 0 and 3 chunks arriving from the server (mid-stream — a
|
||||
leading `0x00` is the negotiation-error marker, ADR-052 §5, so
|
||||
stream_type 0 cannot be first) are ignored without desynchronizing
|
||||
framing, losing stdout data, or blocking the exit. The remaining
|
||||
P13 item (the `TtyAdapter::handle` accept-loop harness) stays
|
||||
deferred to the first transport-level consumer, per the review.
|
||||
|
||||
Verification on the resolution tree: `cargo test` 98 lib (was 95) /
|
||||
`cargo test --all-features` 141 (was 138); clippy (all-targets +
|
||||
wasm32), fmt, wasm check, doc — all clean.
|
||||
|
||||
## Notes
|
||||
|
||||
- Line numbers refer to the tree at `918af40` (review base).
|
||||
|
||||
Reference in New Issue
Block a user