Repository navigation
epoll: the driver read's check and read, closeDriver's DEL and a refused ADD's rollback have tests, with two nil-by-default seams (v1.6.0 polish items) - #1086
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Merging with every required check green (Build, Conformance, Driver Conformance, Lint, Unit, Vulnerability Check). The advisory items are not code results: "CodSpeed Macro Runners" reports action_required because the org's 600 Macro Runner minutes are used up until 2026-10-31, and the "Benchmarks (arm64 macro runner)" job has no runner. Neither is a required check. |
…sed ADD's rollback have tests, with two nil-by-default seams (v1.6.0 polish items) (#1086) * epoll: two nil-by-default test seams in the driver paths (driverRead's check-to-read gap, closeDriver's DEL) and one doc sentence on UnregisterConn No behavior change: each seam is a package var that is nil outside a test, read once where the critical section already is. Refs #885 item 3 Refs #885 item 4 Refs #885 item 23 * test(epoll): the driver read's check and read are one critical section, closeDriver's DEL is inside its claim section, and a refused EPOLL_CTL_ADD is rolled back Refs #885 item 3 Refs #885 item 8 Refs #885 item 23
Test-only, except two nil-by-default seams in the epoll driver paths (
driverRead,closeDriver; one load and one nil test per read, nothing on a request's path) and one doc sentence onUnregisterConn. Four items of the epoll driver tests.Refs #885 item 3
Refs #885 item 4
Refs #885 item 8
Refs #885 item 23
What changed
testHookDriverReadChecked(plain nil var,driver.go), called indriverReadbetween thedc.closedcheck andunix.Read,dc.muheld.TestDriverReadCheckAndReadAreOneCriticalSection885is the port of the standalone loop'sTestReadCheckAndReadAreOneCriticalSection784(#843) to a bareLoop: the hook startsUnregisterConn(A)on a goroutine and waits 300 ms; if it returns, the reader was overtaken between check and read, so the test closes A, moves X (bytes waiting) onto A's number and lets the read go. Not overtaken: it does the close and the reuse afterUnregisterConnreturns and offers the old conn another read turn. Asserts: not overtaken, onRecv got exactly A'sAAAA, X still holds its bytes, onClose onceUnregisterConn's doc: a read that completed before it tookdc.muis still delivered, soonRecvcan run once more, concurrently with or afteronCloseTestRegisterConnRollsBackAFailedEpollAdd885(driver_test.go): a regular file's descriptor, refused byEPOLL_CTL_ADDwith EPERM. Assertserrors.Is(err, unix.EPERM),hasDriverConnsfalse,lookupDrivernil,driverConnsempty,WriteandUnregisterConngiveErrUnknownFD,onClosenever ran. A second arm registers a good conn first and asserts the failed registration leaveshasDriverConnsset and the neighbour in the map. #1058 (ErrUnknownFDon a closing conn) changed the io_uring side; epoll'sWrite/UnregisterConnalready returned it on main 6a6b07f: the head'sRegisterConn,WriteandUnregisterConnare main's code unchanged (this PR touchesdriverRead,closeDriverand doc comments only), and this test passes on the headtestHookCloseDriverDeleted, insidecloseDriver'sdc.musection after the DEL.TestCloseDriverDeletesInsideItsClaimSection885asserts in the hook thatdc.muis held (TryLockfails), the conn is claimed closed, andEPOLL_CTL_MODgives ENOENT; the same MOD beforecloseDrivermust succeed (premise: the ENOENT is the DEL's)Commits:
6a839357(seams and the doc sentence, no behaviour change),5ba32414(the three tests).Base, and what "failing first" can mean here
main is 6a6b07f. The code the tests pin is already right on main, and the two tests of items 3 and 23 need the seams, so on main they do not compile; a compile failure is not accepted as a control. So the seams are the first commit, the tests the second, the tests PASS on the head, and the evidence that they discriminate is the mutants below, each a patch under
patches/applied todriver.gowith the seams in place (gofmtclean, builds, red for the stated reason, not a compile error). Item 8 is test-only and passes on main's production code; its controls are mutants too.Negative controls (arm64 container, golang:1.27.2, kernel 7.0.12-linuxkit)
driver.go)-raceonDriver|RegisterConn|UnregisterConn|771|775|885(114 PASS, 0 FAIL, no DATA RACE, 84 s,logs/r1-pr1-race-head.log, which records the head 5ba3241, a clean tree and thego test -racecommand line; the earlierlogs/pr1-race-driver.logrecorded neither the tree nor that-racewas on)dc.mu.Unlock(), hook,dc.mu.Lock()between the check and the read (#772's m1-toctou)TestDriverReadCheckAndReadAreOneCriticalSection885FAIL:UnregisterConn returned while the reader was between its closed check and its read, A's onRecv got X's bytes (logs/mut-m1-toctou-split.log)closedunderdc.mu(the pre-#710 reader)logs/mut-m2-check-after-read.log)dc.mu.Unlock()(the mutant that passed every 771 and 775 test)TestCloseDriverDeletesInsideItsClaimSection885FAIL:EPOLL_CTL_MOD in the claim section after the DEL: <nil>, want ENOENT(logs/mut-m3-del-after-unlock.log)closeDriverlogs/mut-m4-del-removed.log)hasDriverConnsTestRegisterConnRollsBackAFailedEpollAdd885/no_other_connFAIL:hasDriverConns is still set(logs/mut-m5-no-hasdriverconns-reset.log)Writereturns nil,UnregisterConnreturns nil, onClose ran (logs/mut-m6-no-map-delete.log)hasDriverConns.Store(false)regardless of other conns.../another_conn_registeredFAIL:hasDriverConns was cleared by a failed registration while another driver conn is registered;no_other_connPASSES, so the second arm is what catches it (logs/mut-m7-reset-unconditionally.log)Each mutant leaves the other two tests green (the test of an item goes red, the others stay), so none of the reds is the apparatus. The 300 ms wait of item 3's test is paid by the correct code (0.30 s); a mutant fails it in a few microseconds, so the check is one-sided: a slow scheduler can only hide a mutant, never fail the correct code.
Neighbours fixed in the same file
driver.go,testHookCloseDriverClaimed's comment: "the caller close the descriptor" now says "can close".RULE 74: A/B owed, not run (a pre-merge gate; item 3 cannot be ticked from this PR alone)
The seam in
driverReadis a package-var load and a nil test per read, inside the read's existingdc.musection. I did not time it (no cluster here, and the laptop's Docker VM is not a timing host). Request for the cluster, base6a6b07f9against this head, the same binary build flags, amd64 and arm64:BenchmarkDriverPingPong930(one op is a one-byte write and the loop's read of it, the path the seam is on once per op) andBenchmarkDriverFloodThroughput930(the seam is on every one of up to eight reads per turn), benchstat of base against head. ThecloseDriverseam is on a teardown, not a hot path. Pass: no difference outside the noise of the base's own-count=10.Item 885.3 asks to measure the hook's cost or build-tag it. Neither is done, so item 3 stays open until
BenchmarkDriverPingPong930andBenchmarkDriverFloodThroughput930are benchstat-compared (base 6a6b07f against this head) or the hook is build-tagged. The orchestrator should treat the A/B as a merge gate; items 4, 8 and 23 do not depend on it.What I can show without a timing host is the static cost, from
GOOS=linux go build -gcflags=-Sof base 6a6b07f and this head 5ba3241 (scriptstatic-asm.sh, outputlogs/r1-static-asm.log, thedriverReadassembly inlogs/asm-base-*.txtandlogs/asm-head-*.txt):driverReadgoes from 692 to 724 bytes on amd64 and from 864 to 896 bytes on arm64, with the frame (locals=0x58,locals=0x68) unchanged. With the hook nil the hot path gains a load oftestHookDriverReadCheckedand a test of it:MOVQ,TESTQ,JEQon amd64 (3 instructions),MOVD,CBZon arm64 (2). The branch jumps past the reloads of the receiver and the conn that follow the hook call, so those run only when the hook is set. The seam is inside the existingdc.musection, right before aread(2); only driver-conn reads reach it.testHookDriverQueueWait(loop.go) is the same shape on the run loop. That is instruction counts, not a timing, and it does not replace the A/B.CI
Draft PR, 28 of 28 checks pass on 5ba3241 (the PR's Checks tab; the head is unchanged by round 1); the tests of this PR ran in the amd64 unit shards (ubuntu-latest) and passed; the A/B benchmarks are not run by CI.
Log provenance
The mutant logs of this PR (
logs/mut-m1-*.logtologs/mut-m7-*.log) were written before the lane's container script recorded the tree they ran on (theirhead=field is empty, becausegitcannot resolve a worktree's.gitfile inside the container). Each mutant is a patch underpatches/applied to this head, so the runs are reproducible, but a log alone does not prove which tree it ran on. Logs written in round 1 (logs/r1-*.log) recordhead=, the dirty-file count and the command line.Not run (owed, not passed)
disk-clean.sh --applyhad nothing of its own to reclaim.RULE 74 on the cluster: hook A/B (added 2026-10-20)
Pre-registered timing A/B with an A/A twin (probatorium
celeris-stresstiming mode, run 39446879155, both architectures, 16 counterbalanced blocks per arm and host, pinned to one CPU; planab-prep/PLAN-hookab-amd64-arm64.md, readscluster-reads-20261016/hookab-run-39446879155): arms A and Twin = main 6ab5c28 plus the measurement wrapper (same binary on each host), H = A plus this series' hijack-hook hunk only, D = A plus this series' driver hunks only (the production diffs match the PRs'). Validator PASS on both hosts (x86: msa2-server, arm64: msr1; all arms complete, A and Twin identical binaries).BenchmarkDriverPingPong930/FloodThroughput930were not run (they spin-wait and cannot run at msr1's single pinned CPU): the wrapper's driver shapes are the stand-in, stated in the plan.