Skip to content

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

Merged
FumingPower3925 merged 3 commits into
mainfrom
fix/eptstdrv-polish
Oct 20, 2026
Merged

FumingPower3925 merged 3 commits into
mainfrom
fix/eptstdrv-polish

Conversation

@FumingPower3925

@FumingPower3925 FumingPower3925 commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

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 on UnregisterConn. 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

Item Change
3 testHookDriverReadChecked (plain nil var, driver.go), called in driverRead between the dc.closed check and unix.Read, dc.mu held. TestDriverReadCheckAndReadAreOneCriticalSection885 is the port of the standalone loop's TestReadCheckAndReadAreOneCriticalSection784 (#843) to a bare Loop: the hook starts UnregisterConn(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 after UnregisterConn returns and offers the old conn another read turn. Asserts: not overtaken, onRecv got exactly A's AAAA, X still holds its bytes, onClose once
4 One sentence on UnregisterConn's doc: a read that completed before it took dc.mu is still delivered, so onRecv can run once more, concurrently with or after onClose
8 TestRegisterConnRollsBackAFailedEpollAdd885 (driver_test.go): a regular file's descriptor, refused by EPOLL_CTL_ADD with EPERM. Asserts errors.Is(err, unix.EPERM), hasDriverConns false, lookupDriver nil, driverConns empty, Write and UnregisterConn give ErrUnknownFD, onClose never ran. A second arm registers a good conn first and asserts the failed registration leaves hasDriverConns set and the neighbour in the map. #1058 (ErrUnknownFD on a closing conn) changed the io_uring side; epoll's Write/UnregisterConn already returned it on main 6a6b07f: the head's RegisterConn, Write and UnregisterConn are main's code unchanged (this PR touches driverRead, closeDriver and doc comments only), and this test passes on the head
23 testHookCloseDriverDeleted, inside closeDriver's dc.mu section after the DEL. TestCloseDriverDeletesInsideItsClaimSection885 asserts in the hook that dc.mu is held (TryLock fails), the conn is claimed closed, and EPOLL_CTL_MOD gives ENOENT; the same MOD before closeDriver must 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 to driver.go with the seams in place (gofmt clean, 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)

Mutant (driver.go) Item Result
none (head) 3, 8, 23 all PASS, also with -race on Driver|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 the go test -race command line; the earlier logs/pr1-race-driver.log recorded neither the tree nor that -race was on)
m1-toctou-split: dc.mu.Unlock(), hook, dc.mu.Lock() between the check and the read (#772's m1-toctou) 3 TestDriverReadCheckAndReadAreOneCriticalSection885 FAIL: 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)
m2-check-after-read: hook, read with no lock, then check closed under dc.mu (the pre-#710 reader) 3 same test FAIL: overtaken, X's bytes consumed (logs/mut-m2-check-after-read.log)
m3-del-after-unlock: the DEL moved to just after dc.mu.Unlock() (the mutant that passed every 771 and 775 test) 23 TestCloseDriverDeletesInsideItsClaimSection885 FAIL: EPOLL_CTL_MOD in the claim section after the DEL: <nil>, want ENOENT (logs/mut-m3-del-after-unlock.log)
m4-del-removed: no DEL in closeDriver 23 same test FAIL, same message (logs/mut-m4-del-removed.log)
m5-no-hasdriverconns-reset: the failed ADD does not reset hasDriverConns 8 TestRegisterConnRollsBackAFailedEpollAdd885/no_other_conn FAIL: hasDriverConns is still set (logs/mut-m5-no-hasdriverconns-reset.log)
m6-no-map-delete: the failed ADD does not delete the map entry 8 both arms FAIL: lookup non-nil, Write returns nil, UnregisterConn returns nil, onClose ran (logs/mut-m6-no-map-delete.log)
m7-reset-unconditionally: hasDriverConns.Store(false) regardless of other conns 8 .../another_conn_registered FAIL: hasDriverConns was cleared by a failed registration while another driver conn is registered; no_other_conn PASSES, 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 driverRead is a package-var load and a nil test per read, inside the read's existing dc.mu section. I did not time it (no cluster here, and the laptop's Docker VM is not a timing host). Request for the cluster, base 6a6b07f9 against this head, the same binary build flags, amd64 and arm64:

go test -run '^$' -bench 'BenchmarkDriver(PingPong|FloodThroughput)930' -benchtime=3s -count=10 ./internal/engine/epoll

BenchmarkDriverPingPong930 (one op is a one-byte write and the loop's read of it, the path the seam is on once per op) and BenchmarkDriverFloodThroughput930 (the seam is on every one of up to eight reads per turn), benchstat of base against head. The closeDriver seam 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 BenchmarkDriverPingPong930 and BenchmarkDriverFloodThroughput930 are 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=-S of base 6a6b07f and this head 5ba3241 (script static-asm.sh, output logs/r1-static-asm.log, the driverRead assembly in logs/asm-base-*.txt and logs/asm-head-*.txt): driverRead goes 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 of testHookDriverReadChecked and a test of it: MOVQ, TESTQ, JEQ on amd64 (3 instructions), MOVD, CBZ on 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 existing dc.mu section, right before a read(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-*.log to logs/mut-m7-*.log) were written before the lane's container script recorded the tree they ran on (their head= field is empty, because git cannot resolve a worktree's .git file inside the container). Each mutant is a patch under patches/ 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) record head=, the dirty-file count and the command line.

Not run (owed, not passed)

  • The A/B above, amd64 native timing, real kernels other than 7.0.12-linuxkit, the cluster hosts.
  • The two Opus review lenses (this lane had no way to spawn them).
  • Free disk was 40-42 GiB during the first runs and 53-56 GiB during round 1 (the lane rule asks for more than 60); disk-clean.sh --apply had 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-stress timing mode, run 39446879155, both architectures, 16 counterbalanced blocks per arm and host, pinned to one CPU; plan ab-prep/PLAN-hookab-amd64-arm64.md, reads cluster-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).

  • Result: every shape of both arms is CLEAN on both architectures (own and control shapes; rate, p99, CPU, allocations and errors). Ratios of the ns-per-op to the mean of A and Twin: hijack hunk (H) HijackSync 1.0035 [0.998, 1.009] x86 / 1.0054 [0.995, 1.012] arm64, HijackAsyncInline 1.0026 / 0.9978; driver hunks (D) DriverReadBare1B 1.0020 / 1.0048, DriverReadBareFlood 0.9991 / 1.0025, DriverCloseBare 0.9995 / 0.9990, PingPong1B 0.9932 [0.948, 1.039] / 0.9833 [0.968, 1.026] (one load cannot resolve 3% on PingPong1B; the three bare driver shapes resolve to about +-0.5%). Promotions and errors 0 in every arm.
  • Reading: NO-REGRESSION on both architectures for the nil-hook path (the hooks are unset in every shape). It excludes a cost of more than about 1% on the hijack and bare driver paths at this resolution; the literal BenchmarkDriverPingPong930 / FloodThroughput930 were 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.

…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
…n, 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
@FumingPower3925 FumingPower3925 added this to the v1.6.0 milestone Oct 20, 2026
@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: goceleris/celeris/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: e371519c-3c96-446d-9bb4-131220a56fe8
📥 Commits

Reviewing files that changed from the base of the PR and between c5206ab and 14751a2.

📒 Files selected for processing: 3
  • internal/engine/epoll/driver.go
  • internal/engine/epoll/driver_critical_sections_885_linux_test.go
  • internal/engine/epoll/driver_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

Adds epoll driver test hooks and regression tests for failed registration cleanup, read and unregistration ordering, and connection closure ordering.

Changes: Epoll driver operations

Layer / File(s) Summary
Failed registration rollback
internal/engine/epoll/driver_test.go
Tests verify that a failed EPOLL_CTL_ADD removes the failed connection state without affecting existing registrations.
Read and unregister ordering
internal/engine/epoll/driver.go, internal/engine/epoll/driver_critical_sections_885_linux_test.go
Adds a read synchronization hook and documents callback ordering. A Linux test checks that unregistration cannot cause a read to consume bytes from a reused descriptor.
Close and epoll deletion ordering
internal/engine/epoll/driver.go, internal/engine/epoll/driver_critical_sections_885_linux_test.go
Adds a close synchronization hook. A Linux test checks that the connection is marked closed and removed from epoll while its lock is held.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Other

Possibly related issues

  • Issue 885: The tests cover the read ordering, failed registration rollback, and close ordering described in the change summary.
  • Issue 890: The PR adds lifecycle ordering regression tests with synchronization around the read and close paths.
  • Issue 891: The UnregisterConn documentation now describes read and close callback ordering.
  • Issue 892: The close-path test checks the EPOLL_CTL_DEL claim section, but the PR does not run the requested driver benchmarks.
Merge Risk: ⚪ Minimal · up to 14751

The added regression tests have no identified merge-blocking issue; the change is ready for normal merge checks.

Pre-merge checks: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Title check Warning The title describes the epoll test changes, but it does not use the required Conventional Commit format because epoll is not an allowed type. Use an allowed type, such as test(epoll): add driver critical-section and rollback tests. Add an issue reference if this PR is considered a fix for one.
Passed Passed checks: 3 passed
Check name Status Explanation
Description check Passed The description is directly related to the epoll driver tests, documentation change, test seams, and reported validation results.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Oct 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@FumingPower3925
FumingPower3925 marked this pull request as ready for review October 20, 2026 16:46
@FumingPower3925

Copy link
Copy Markdown
Contributor Author

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.

@FumingPower3925
FumingPower3925 merged commit 352fbcf into main Oct 20, 2026
28 checks passed
@FumingPower3925
FumingPower3925 deleted the fix/eptstdrv-polish branch October 20, 2026 17:11
FumingPower3925 added a commit that referenced this pull request Oct 21, 2026
…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
FumingPower3925 pushed a commit that referenced this pull request Oct 21, 2026
@FumingPower3925 FumingPower3925 mentioned this pull request Oct 25, 2026
39 of 56 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants