Renames the written-usage rule into practice across the contract surface: docs/ (architecture, research, plans, sdd), tasks/ (all 43 task files, taskgraph validate passes), AGENTS.md, README.md, CHANGELOG.md, and the .opencode/agents/ prompts. Every replacement is a sense-preserving rewrite (colons, semicolons, commas, parentheses, sentence splits, varied), never a mechanical swap; no spaced hyphens were substituted. Enum fields, frontmatter, tables, wire data, and cited identifiers survive verbatim; en dashes inside numeric ranges keep their meaning. Verification: taskgraph validate (43 tasks), repo grep clean (single exempt hit: the verification command's own search pattern in AGENTS.md), cargo test, clippy --all-targets -D warnings, fmt --check, doc all green.
185 lines
9.3 KiB
Markdown
185 lines
9.3 KiB
Markdown
---
|
||
id: w4-git-exit-close
|
||
name: "Wave 4: git row hardening (status-before-close by construction, wave-3 gate booked item 1)"
|
||
status: completed
|
||
depends_on: [w3-git-mapping]
|
||
scope: narrow
|
||
risk: medium
|
||
impact: component
|
||
level: implementation
|
||
---
|
||
|
||
## Description
|
||
|
||
Close the wave-3 gate's only structural booked finding
|
||
(`review-wave-3` §Booked findings item 1): the git row's
|
||
status-before-close is honored **by scheduling, not by construction**.
|
||
Today `serve_duplex_resolved` splits the channel stream internally and
|
||
its session halves drop when the serve returns, so russh's
|
||
`ChannelCloseOnDrop` rx-half emits a server→client `CHANNEL_CLOSE`
|
||
*inside* the seam call, before `serve_git_row`'s explicit
|
||
`exit_status_request`. The ordering currently holds (the task's
|
||
continuation queues the report in the same poll, ahead of the drop's
|
||
send) and every wire row passes, but a full `Sender<Msg>` buffer at
|
||
the serve tail could order the implicit close first and silently drop
|
||
the report (russh no-ops `exit_status_request` for an already-closed
|
||
channel), a silent exit-report loss on a published row.
|
||
|
||
The fix wants a seam/shape change (decide and record which):
|
||
|
||
- split the channel halves in `serve_git_row` and pass them into the
|
||
seam (the report then rides a half the seam never closes), or
|
||
- queue the exit report while the stream is alive (the report's send
|
||
ordered before any close-contracting drop by construction), or
|
||
- another shape that makes the ordering structural rather than
|
||
scheduler-luck; the gate's reasoning and the two named options are
|
||
the starting points, not the bounds.
|
||
|
||
Constraints: every existing git wire row stays green (no behavior
|
||
change on the happy path, since this is ordering hardening, not
|
||
semantics
|
||
change); the spawn-and-return constraint (ADR-006 decision 4) holds,
|
||
because the reporting still rides the spawned task; the tty rows are NOT in
|
||
this class (their report precedes any close-contracting drop in one
|
||
task, which the gate verified; don't touch them). New wire row(s) pinning
|
||
the construction: a saturated `Sender<Msg>` buffer at the serve tail
|
||
(the failure scenario made real) still delivers the exit report,
|
||
the by-construction property proven on the wire, not by reasoning.
|
||
|
||
## Acceptance Criteria
|
||
|
||
- [ ] The shape decided + recorded (module docs carry the reasoning);
|
||
the git row's status-before-close holds by construction, not
|
||
scheduling
|
||
- [ ] Every existing git wire row green (no happy-path behavior
|
||
change); the spawn-and-return constraint intact
|
||
- [ ] New wire row(s): the saturated-buffer scenario delivers the
|
||
exit report (the by-construction property pinned)
|
||
- [ ] `cargo test` (+ per-feature + `--all-features`), clippy ×2
|
||
(`-D warnings`), fmt, `cargo doc --no-deps` pass
|
||
|
||
## References
|
||
|
||
- tasks/implementation/review-wave-3.md §Booked findings item 1 (the
|
||
full finding + the two named fix shapes)
|
||
- docs/architecture/mapping.md (the git exec row, exit-status both
|
||
directions), docs/architecture/decisions/006-mapping-bridge-and-dispatch.md
|
||
(decision 4, spawn-and-return)
|
||
- src/git_map.rs, src/serve.rs (`report_exit_status_then_close`, the
|
||
wave-2 reporting mechanics this hardening composes with), alkgit
|
||
0.1.0 `serve_duplex_resolved` (the seam whose half-ownership is the
|
||
question)
|
||
|
||
## Notes
|
||
|
||
### The shape decided (2026-10-29)
|
||
|
||
Went with the first named option, **splitting the channel halves in
|
||
`serve_git_row` and passing them into the seam**, refined by one russh
|
||
0.65 reality: `russh::channels::io::{ChannelRx, ChannelTx}` are
|
||
*private* at this pin, so the split-halves duplex is rebuilt from the
|
||
public API in `src/git_map.rs`:
|
||
|
||
- **The reader** is `git_map`'s own
|
||
[`ChannelHalveReader`]: the owned `ChannelReadHalf`
|
||
(`channel.split()`), a `poll_read` state machine whose observation
|
||
semantics repin russh's private `ChannelRx` *without* the
|
||
`ChannelCloseOnDrop` wrapper, so `Data` fills (partial reads
|
||
buffered), `ExtendedData`/other non-data messages swallowed and
|
||
re-read (the `_`-arm parity), `Eof` ends the stream, the peer's
|
||
`Close` leaves the reader to the channel's sender-death EOF
|
||
(`wait()` → `None`). The `wait()` access is *future-owned*: each
|
||
read round-trips the half through a boxed future that parks it
|
||
inside `ChannelReadHalf::wait` and hands it back with the observed
|
||
message, fully owned (`'static`-usable, the seam's `T` bound)
|
||
across every park/buffer/drop state, no extra task, and **no
|
||
`Drop` path ever messages the wire**.
|
||
- **The writer** is the write half's `ChannelTx` via
|
||
`make_writer_ext(None)`. Its value is `'static`-free of the half
|
||
(clones only) but the Rust 2024 capture rules give the opaque a
|
||
phantom of the half's borrow; a trait-object erasure
|
||
(`Box<dyn AsyncWrite + Unpin + Send>`) drops the phantom, leaving
|
||
the seam's `'static` bound satisfiable without parking the half.
|
||
`poll_shutdown` still sends EOF (exactly `into_stream`'s).
|
||
|
||
The result: **nothing the seam drops ever messages the wire**, with the
|
||
report's `exit_status_request` send is the sole close-contracting
|
||
sender, and its queue position ahead of the explicit close holds by
|
||
construction (sequential awaited sends on the one `Handle`),
|
||
saturated buffer or not. The hazard's premise (russh's
|
||
`ChannelCloseOnDrop` `Drop` spawning a `CHANNEL_CLOSE` send that can
|
||
win a queue position on the bounded `Sender<Msg>`
|
||
(`event_buffer_size`, default 10) before the report's, is *removed*,
|
||
not raced.
|
||
|
||
### The wire row (the by-construction property pinned)
|
||
|
||
`tests/git_mapping.rs::backlogged_seam_tail_still_reports_exit_status_before_close`,
|
||
the finding's failure scenario made real on the wire: the serve's
|
||
answer volume (300 one-pkt-line ref answers) exceeds the entire
|
||
outbound pipeline via the new client-direction data gate
|
||
(`tests/common/harness.rs::DataGate` + the harness handler's `data`
|
||
hook, which russh's client loop awaits inline): the armed gate freezes
|
||
the client's loop → the transport duplex fills (`DUPLEX_CAPACITY`) →
|
||
the server's flush pends → russh's bounded message queue fills → the
|
||
serve's remaining answer writes pend (the serve tail blocked). The
|
||
gate's release then drives the serve tail through the just-unwinding
|
||
contention, and the row pins: the whole backlogged answer drains
|
||
**through the same queue the report rides**, then exit-status(0), then
|
||
close; the order is asserted, the exit code asserted.
|
||
|
||
**The row falsifies the old shape.** With a temporary one-line revert
|
||
of `serve_git_row` to `channel.into_stream()` the row FAILED
|
||
**15/15** runs, as the implicit close won the frozen-drain queue race
|
||
("expected exit-status(0), got Some(Close)"), the silent report loss
|
||
the gate booked. With the split-halves shape: **10/10 green**. So the
|
||
row is a two-sided pin: deterministic red under the hazard, green by
|
||
construction without it.
|
||
|
||
Test-vehicle additions (test surface, freely versioned): the doubles'
|
||
per-repo serve gate (`tests/common/git_doubles.rs`: `install_gate`, a
|
||
race-safe double-check `MemGate`, gating `GitRefs::list_refs`, so the
|
||
row schedules the serve's stages against prepared wire state) and the
|
||
harness's `DataGate` + `drive_with_data_gate` (the `drive*` bodies
|
||
factored into the shared `mount_and_connect` core; existing wrappers'
|
||
signatures unchanged).
|
||
|
||
### Constraints check
|
||
|
||
- **Every existing git wire row green**, with the full git suite
|
||
(`tests/git_mapping.rs`, 12 rows) and every other loopback suite:
|
||
no behavior change on the happy path (the wire order the rows pin,
|
||
i.e. data, exit-status, close, is identical under both shapes when
|
||
the race doesn't fire).
|
||
- **Spawn-and-return intact** (ADR-006 decision 4): the reporting
|
||
still rides the spawned serving task through the cloned
|
||
`Session::handle()`; `serve.rs`'s mechanics untouched.
|
||
- **The tty rows untouched** (not in this class; the gate verified that).
|
||
- **No API/limits change**: `event_buffer_size` stays russh's default
|
||
posture (the adapter docs' recorded default posture preserved); the
|
||
row pins the default's value with a note that a russh default
|
||
change re-reads the row's freeze-settle yields, not its assertions.
|
||
|
||
## Summary
|
||
|
||
**Completed.** The git row's status-before-close now holds **by
|
||
construction**: `serve_git_row` no longer hands russh's
|
||
close-on-drop `ChannelStream` (`into_stream()`) to the seam; the
|
||
seam rides a split-halves duplex (`git_channel_duplex` →
|
||
`ChannelHalveReader` over the owned read half + the write half's
|
||
`ChannelTx` via the capture-erased trait object) where **no drop
|
||
path ever messages the wire**, making the report's send the sole
|
||
close-contracting sender and the report-before-close order
|
||
unconditional (russh 0.65's private `ChannelRx`/`ChannelTx` noted;
|
||
the semantics repinned from the public API). New wire row pins the
|
||
by-construction property on a genuinely saturated tail (volume flood
|
||
→ frozen client loop → duplex → bounded message queue; gate release
|
||
→ the tail through the unwinding contention), and it also falsifies
|
||
the old shape deterministically (15/15 red under a temporary
|
||
`into_stream()` revert, 10/10 green with the fix). Doubles gained a
|
||
race-safe per-repo serve gate; the harness gained the client-side
|
||
data gate + `drive_with_data_gate` (the `drive*` cores factored,
|
||
signatures unchanged). Verification: `cargo test` (default),
|
||
`--all-features`, and 6 mixed per-feature builds all green; clippy ×2
|
||
`-D warnings` clean; fmt clean; `cargo doc --no-deps` warning-free;
|
||
`taskgraph validate` clean (35 tasks). |