Co-Authored-By: Claude Opus 5.5 <[email protected]>
This commit is contained in:
Claude Opus 5.5
@@ -0,0 +1,116 @@
|
||||
# Row 100 (#1594) round 1: git socket hardening and refusal tests
|
||||
|
||||
Brief: docs/plans/2026-10-12_pm-test-fixes.md, section "Row 93 follow-up: git socket hardening and refusal tests" (commit 8fda9611, blob 017ac786).
|
||||
BASE: 8e0fbca420a3161ce98c5e0f3910f500daabebba. Candidate: build.patch against BASE; candidate-manifest.sha256 lists the files.
|
||||
|
||||
## What changed
|
||||
|
||||
- packages/seat/src/working.mjs. A git past GIT_TIMEOUT_MS gets SIGTERM, then SIGKILL after GIT_GRACE_MS (5 s). git removes its lockfiles on SIGTERM. A rollback whose update-ref fails refuses rollback-failed, which outranks record's own error. push and review take `limits` so the tests can shorten both timers.
|
||||
- packages/cli/src/git-socket.mjs.
|
||||
- readBody has a total deadline, `bodyDeadline(size) = 30000 + ceil(size / 256)` ms (161 s at the 32 MiB cap), beside the 30 s idle timeout. Both refuse body-timeout.
|
||||
- The bundle send to a reviewer is a loop, `send()`. It used to be pipeline(), whose ERR_STREAM_PREMATURE_CLOSE became git-failed. A close while chunks are still to go is fetch-aborted, and so is a close after a write of a queued chunk failed (EPIPE). A close after every chunk left with no failed write is a served fetch, because the runner reads exactly `size` bytes and hangs up, often before the host's loop sees the file's end or before 'finish'.
|
||||
- git-failed and rollback-failed go to the log as `git socket: <code>: <message>`.
|
||||
- close() answers host-closing to a connection still reading its request line, and drops the rest as before.
|
||||
- `maxActive` and `bodyDeadline` are options for the tests.
|
||||
- packages/harness/src/git-tools.mjs. fetch_review's `have` lists every clone's working refs, newest first, before any of origin's, and stops at HAVE_MAX.
|
||||
- packages/seat/README.md: rollback-failed, the SIGTERM/SIGKILL timing, and the lock's one-launcher-per-dataRoot assumption.
|
||||
- packages/cli/README.md: body-timeout, what nothing-to-fetch, unknown-project and fetch-aborted mean to a reviewer, the log lines, and host-closing.
|
||||
|
||||
## Tests
|
||||
|
||||
| Brief item | Test |
|
||||
|---|---|
|
||||
| not-a-role | git-socket.test: no role, then another business |
|
||||
| host-closing | git-socket.test: close() while a request line is half sent |
|
||||
| git-busy | git-socket.test: maxActive 2, a third connection |
|
||||
| root-unreadable | working.test: a root that doesn't exist |
|
||||
| unknown-project | git-socket.test: a review naming a project the business lacks |
|
||||
| timed-out push leaves no lock | working.test: a hook holds the ref lock, SIGTERM, no .lock, next push lands |
|
||||
| SIGKILL escalation | working.test: a git that ignores SIGTERM |
|
||||
| rollback-failed | working.test (delete and previous-ref cases) and git-socket.test (logged, trail) |
|
||||
| trickling client | git-socket.test: body-timeout within the deadline, slot freed |
|
||||
| fetch-aborted | git-socket.test: a reviewer leaves mid-bundle; no git-failed, no log |
|
||||
| a served fetch isn't fetch-aborted | git-socket.test: the review loop now closes the socket and checks action.refused is empty; a raw reviewer takes a 1 MiB bundle and hangs up at once, ten times; `send()` against two stand-in sockets, one that never finishes and one whose queued write fails EPIPE |
|
||||
| have over 32 refs | harness git-tools.test: 53 refs over two clones |
|
||||
|
||||
## Mutants
|
||||
|
||||
16 mutants, each in `agents/darkwing/work/queue-100/mutants.sh`, run with `mut.mjs` (one exact match, else exit 3) and `run.py` (own process group, killpg at 240 s). All 16 killed:
|
||||
|
||||
```
|
||||
sigkill-first: killed (ℹ fail 1, ✖ a git past its time gets SIGTERM, leaves no ref lock, and the next push lands (10291.116)
|
||||
no-escalation: killed (TIMEOUT after 240 s, ✖ a git that ignores SIGTERM gets SIGKILL once the grace period is over (20098.619138ms))
|
||||
no-rollback-check: killed (ℹ fail 2, ✖ a record that fails and can't be undone refuses rollback-failed, which the host logs (39)
|
||||
rollback-not-logged: killed (ℹ fail 1, ✖ a record that fails and can't be undone refuses rollback-failed, which the host logs (41)
|
||||
no-root-check: killed (ℹ fail 2, ✖ a root that can't be read refuses root-unreadable (35.076601ms))
|
||||
no-role-check: killed (ℹ fail 1, ✖ an identity with no role, or of another business, refuses not-a-role and moves no ref (2)
|
||||
no-business-check: killed (ℹ fail 1, ✖ an identity with no role, or of another business, refuses not-a-role and moves no ref (2)
|
||||
no-project-check: killed (ℹ fail 1, ✖ a review of a project the business no longer has refuses unknown-project (259.074398ms))
|
||||
no-busy-check: killed (ℹ fail 2, ✖ past maxActive connections the socket refuses git-busy, and a hung-up connection frees i)
|
||||
no-closing-answer: killed (ℹ fail 1, ✖ close() answers host-closing to a connection still sending its request line (180.301962m)
|
||||
no-body-deadline: killed (ℹ fail 1, ✖ a bundle that trickles in refuses body-timeout at its deadline and frees its slot (5212.)
|
||||
abort-as-git-failed: killed (ℹ fail 2, ✖ a reviewer gone before its bundle was sent is filed fetch-aborted, not as the host's fau)
|
||||
close-is-aborted: killed (ℹ fail 2, ✖ a reviewer that hangs up the moment it has the whole bundle was served, not aborted (508)
|
||||
errored-ignored: killed (ℹ fail 1, ✖ send: a close after every byte left is served; one that fails a queued write is fetch-ab)
|
||||
have-origin-first: killed (ℹ fail 1, ✖ fetch_review's have lists the working refs before origin's, and no more than HAVE_MAX (1)
|
||||
have-uncapped: killed (ℹ fail 1, ✖ fetch_review's have lists the working refs before origin's, and no more than HAVE_MAX (1)
|
||||
```
|
||||
|
||||
no-escalation hits the 240 s cap: its test fails at its own 20 s timeout, and the wrapper git that ignores SIGTERM keeps node --test alive until run.py kills the group. The sweep after each run found none of my processes left.
|
||||
|
||||
### What the mutants found
|
||||
|
||||
The first cut of `send()` waited for 'finish' and filed any earlier close as fetch-aborted. abort-as-git-failed showed it: the review loop test logged `git-failed: the reviewer left before the bundle was sent` for both of its fetches, so with the real code each successful fetch was on the trail as `reviewer fetch-aborted`. The review loop test checked the log but not action.refused, so it passed. Instrumenting showed the close usually comes before the loop reads the file's end, not only before 'finish'. The fix is above; the review loop now checks action.refused after close(), and the three tests in the table cover the race. A mutant that counted bytes (`sent === size`) survived as an equivalent: the loop's `destroyed` check already refuses a close with chunks to go, so I dropped the counter.
|
||||
|
||||
## Gate
|
||||
|
||||
Run in a clean worktree at BASE with the candidate applied and node_modules symlinked, 16:49:50Z to 16:54:16Z on 2026-10-13, by `agents/darkwing/work/queue-100/gate.sh` (test-task under DOCKER_HOST=unix:///nonexistent). Every suite exited 0 with no failures:
|
||||
|
||||
```
|
||||
bus rc=0 16:49:50-16:49:54 ℹ pass 127 ℹ fail 0 ℹ skipped 0 0 marks
|
||||
business rc=0 16:49:54-16:49:55 ℹ pass 67 ℹ fail 0 ℹ skipped 0 0 marks
|
||||
cli rc=0 16:49:55-16:50:12 ℹ pass 146 ℹ fail 0 ℹ skipped 0 0 marks
|
||||
control-board rc=0 16:50:12-16:50:13 ℹ pass 124 ℹ fail 0 ℹ skipped 0 0 marks
|
||||
conversation rc=0 16:50:13-16:51:01 ℹ pass 182 ℹ fail 0 ℹ skipped 0 0 marks
|
||||
discord rc=0 16:51:01-16:51:03 ℹ pass 178 ℹ fail 0 ℹ skipped 0 0 marks
|
||||
harness rc=0 16:51:03-16:51:14 ℹ pass 70 ℹ fail 0 ℹ skipped 0 0 marks
|
||||
mosaic rc=0 16:51:14-16:51:16 ℹ pass 69 ℹ fail 0 ℹ skipped 0 0 marks
|
||||
queue rc=0 16:51:16-16:51:45 ℹ pass 150 ℹ fail 0 ℹ skipped 0 0 marks
|
||||
runs rc=0 16:51:45-16:51:45 ℹ pass 43 ℹ fail 0 ℹ skipped 0 0 marks
|
||||
seat rc=0 16:51:45-16:51:50 ℹ pass 109 ℹ fail 0 ℹ skipped 0 0 marks
|
||||
tasks rc=0 16:51:50-16:51:56 ℹ pass 90 ℹ fail 0 ℹ skipped 0 0 marks
|
||||
webui rc=0 16:51:56-16:52:42 ℹ pass 107 ℹ fail 0 ℹ skipped 0 0 marks
|
||||
test-auth rc=0 16:52:42-16:52:42 selftest: 15 passed, 0 failed
|
||||
test-conductor rc=0 16:52:42-16:52:47 selftest: 17 passed, 0 failed
|
||||
test-config rc=0 16:52:47-16:52:48 selftest: 24 passed, 0 failed
|
||||
test-discord rc=0 16:52:48-16:52:55 discord suite: 66 passed, 0 failed
|
||||
test-extension-package rc=0 16:52:55-16:52:56 extension package selftest: 18 passed, 0 failed
|
||||
test-foundation rc=0 16:52:56-16:53:32 selftest: 44 passed, 0 failed
|
||||
test-queue rc=0 16:53:32-16:54:03 queue suite: 27 passed, 0 failed
|
||||
test-release rc=0 16:54:03-16:54:14 selftest: 14 passed, 0 failed
|
||||
test-task rc=0 16:54:14-16:54:16 selftest: 26 passed, 0 failed
|
||||
ledger rc=0 run separately: pass 78, fail 0
|
||||
```
|
||||
|
||||
packages/ledger has tests but no package.json, so gate.sh prints a MODULE_NOT_FOUND line and "ledger: no test script" for it. I ran `node --test packages/ledger/tests/` on its own; 78 passed. The ledger tests don't touch the row's files.
|
||||
|
||||
The cli suite also passed eight runs in a row after the `send()` fix (five before the counter came out, three on the final code).
|
||||
|
||||
BASE..origin/refactor (a50c9d23a) touches none of the eight manifest paths. build.patch applied to a fresh BASE checkout reproduces candidate-manifest.sha256.
|
||||
|
||||
## Choices
|
||||
|
||||
- host-closing comes from the waiting map in close(). The `if (closed)` check after readHeader stays as a guard; with the map it is unreachable in practice.
|
||||
- body-timeout is a new code for both timeouts. The idle timeout refused invalid-request before. broker.refuse takes any `/^[a-z-]{1,64}$/` code, so there is no schema change. The 30 s idle path has no test of its own; the deadline test uses the same done().
|
||||
- fetch-aborted isn't logged: a reviewer leaving is not a host fault.
|
||||
- `have` sorts by committer date, newest first, inside each remote.
|
||||
|
||||
## Not owned, for the reviewers
|
||||
|
||||
roles/reviewer.md and the harness README and tools.mjs are what a reviewer reads, and row 100 doesn't own them. The nothing-to-fetch text is in packages/cli/README.md. Pointing roles/reviewer.md at it is a follow-up.
|
||||
|
||||
## Follow-ups (not in this row)
|
||||
|
||||
- packages/seat/src/checkout.mjs git() and the harness runner's git() still use spawn's timeout with SIGKILL.
|
||||
- The kill goes to the direct git child, not its process group; a hook or helper git spawns may outlive it.
|
||||
- startHost's host.json single-host check isn't atomic (the README says so).
|
||||
File diff suppressed because it is too large
Load Diff
@@ -0,0 +1,8 @@
|
||||
9137d8fd6ff173d4f04b4fdc2f4caed1ab4492df9c18d63aa4fd26639d1dad49 packages/cli/README.md
|
||||
57b401ce3782ae32cf60877913c18b6b4d6db6b5a499498abd6c26ddc5ce1b58 packages/cli/src/git-socket.mjs
|
||||
8b6c3f9c575b026081c737cce1a0c1f1cd2eff4394092821719ba03e01a659a7 packages/cli/tests/git-socket.test.mjs
|
||||
f07a62bbe53a237ac8aceab0760f5332a1f1cb6f629cfb2822747ba2a559b9fb packages/harness/src/git-tools.mjs
|
||||
15dccbb011be0ba9763afc47e7b423a51de6ccac8c90659eb3660798470ada27 packages/harness/tests/git-tools.test.mjs
|
||||
e1e783be589085801acbf5f536b848d8113ce3e7a7bb4e44c0933c04c604ac41 packages/seat/README.md
|
||||
d23a4377a50fa8e63c8396a3e4fe35ca5bf9f3c114fe22be55e1be3c49a635c1 packages/seat/src/working.mjs
|
||||
b0654288c7ccaed52ee2e8e15d7cd8a1efb2fe8414a838491dad338c4d0c9b8b packages/seat/tests/working.test.mjs
|
||||
Reference in New Issue
Block a user