Repository navigation
test: hold a toggled test upstream's port while it is down - #718
Conversation
toggledUpstream.down closed its listener and released the port. On a busy host another server could take the port and answer 404 "repository not found", a permanent push failure, so TestLocalHookForwardBackoffSurvivesRestart saw no retry scheduled; up could also fail to listen again. A socket bound to the address with SO_REUSEPORT, that never listens, now holds the port for the whole test: dials while down still get "connection refused", and no other server can take the port. Change-Id: I5b1bb2540ccc6eff39fe21715a7b9eed46265827 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e32dd34e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…SOCK_CLOEXEC x/sys/unix has no SOCK_CLOEXEC on Darwin, so the githttp tests did not compile on macOS. CloseOnExec marks the descriptor instead. Change-Id: Ic0741b226e9e60ef57576fdd6df28929e7e292f4 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…and sync internal/sync's httpUpstream released its port on down exactly like githttp's toggledUpstream, and four local-schedule tests take it down and bring it back. internal/testutil/heldport now holds the port for both: Listen binds a second SO_REUSEPORT socket that never listens, and Relisten listens on the address again. httpUpstream.up no longer retries its listen. TestRelistenAfterClose replaces the githttp-only test. Change-Id: Id4502fbcd59520c58d562de6fb738d4a7270b785 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
… socket under ForkLock A socket that also sets SO_REUSEPORT and binds the exact port (on Linux, only from the same user) can still share a held port, so the docs and comments now say so instead of "no other process". The held socket is created and marked close-on-exec while syscall.ForkLock is held, as the standard library does where SOCK_CLOEXEC is missing, so no child process started in between inherits it. Change-Id: I169fb1c1608dc5a648d0b8afb779a67f30973ba9 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Removing unix.CloseOnExec turned no test red. hold now returns its descriptor, and TestHeldSocketIsCloseOnExec checks FD_CLOEXEC on it. Change-Id: I0a8df5ab88d3add939a70106246555e878fdcabf Signed-off-by: Thomas Kosiewski <tk@coder.com>
…tream-port Change-Id: I61ba7223f5b223f33f078cffcc661e9b33d9fb38 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…tream-port Change-Id: I07fe6515bab79e55bff2fc07d08591beb470aad6 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
On macOS a SYN goes to the first bound socket that matches, listening or not (XNU inserts each bound pcb at the head of its hash chain), so the held socket could take the upstream's connections and dials would time out while it is up. hold and the SO_REUSEPORT listener option move to a Linux-only file. Elsewhere Listen and Relisten are plain listeners that hold nothing, as before heldport. The heldport tests run on Linux only, and the docs say so; the README no longer cites the close-on-exec test for the ForkLock claim, which no test covers. Change-Id: I2e156842c2a5b26b8eb114dffca65fcb6233c987 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…tream-port Change-Id: I229aea29f84262adad22cdae50e2452d38fe5649 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Why
Tests that take an upstream down and bring it back must test the sync lanes' retry after an outage, and must not fail because of other processes on the host.
TestLocalHookForwardBackoffSurvivesRestartfailed in a local S3-WAL lane run while #716 was being validated. The forward run ran (PushRunCount:1), but git reportedrepository 'http://127.0.0.1:36635/up/mirror.git/' not found. A 404 is a permanent push failure, so the lane scheduled no retry, and the test failed withattempt=0.The test's upstream (
toggledUpstreamininternal/githttp) went down by closing its listener, which released the port. Another server on the host took the port and answered 404. For the same reason,upcould fail to listen on the port again.httpUpstreamininternal/sync/local_schedule_test.gohas the same down and up, and four local-schedule tests take it down and bring it back.Fix
A new test helper,
internal/testutil/heldport, keeps the port while the upstream is down, on Linux only:Listenlistens on a fresh loopback port withSO_REUSEPORT. It also binds a secondSO_REUSEPORTsocket to the port, and that socket never listens and stays bound until the test ends.Relistenlistens on the same address again, next to the held socket.SO_REUSEPORTand binds that exact port (on Linux, only from the same user), so an ordinary server cannot take it.Off Linux,
ListenandRelistenare plain listeners and hold nothing, as before this PR. On macOS a bound socket that does not listen can receive the port's connections (XNU hands a SYN to the first bound socket that matches), so a held port would make dials time out while the upstream is up. The twoheldporttests run on Linux only.toggledUpstreamandhttpUpstreamnow useheldport.httpUpstream.upno longer retries its listen for 5 s.The change is test-only. It uses
golang.org/x/sys/unix, whichinternal/githttp/hook.goalready imports.unix.CloseOnExecmarks the held socket whilesyscall.ForkLockis held, as the standard library does whereSOCK_CLOEXECis missing, so no child process started in between inherits it.Tests
TestRelistenAfterClose(new,internal/testutil/heldport) checks that a dial is refused after the listener closes, that another listener cannot bind the address, and thatRelistenaccepts connections again. Without the held socket, it fails:another listener could bind 127.0.0.1:37049 while the port was held.TestHeldSocketIsCloseOnExec(new) checks that the held socket hasFD_CLOEXEC. Withoutunix.CloseOnExec, it fails:held socket flags = 0x0, want FD_CLOEXEC.TestLocalHookForwardBackoffSurvivesRestartpasses withOPENSTORAGE_TEST_BACKEND=localand withOPENSTORAGE_TEST_BACKEND=s3wal.go test ./internal/syncpasses on the default backend and withOPENSTORAGE_TEST_BACKEND=s3wal.go vetpasses for./internal/sync/,./internal/githttp/and./internal/testutil/...withGOOS=linux,GOOS=darwinandGOOS=freebsd.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh