Skip to content

refactor(gitlimit): hold a shared lease in every git child of a cache repository - #768

Merged
ThomasK33 merged 16 commits into
mainfrom
issue490-lease
Nov 8, 2026
Merged

ThomasK33 merged 16 commits into
mainfrom
issue490-lease

Conversation

@ThomasK33

@ThomasK33 ThomasK33 commented Nov 7, 2026 •

Copy link
Copy Markdown
Member

Why

ThomasK33 asked to fix the open coder/openstorage issues and land them on main. For #490 that means a cache repository hit by a stale ref lock repairs itself without an operator, when no git that serve started can own the lock.

Deleting a ref lock is safe only when no such git is alive. Today serve cannot prove that. This PR adds the proof and deletes nothing. It is the first of three PRs. Next come the quarantine, the identity check and the counter as a function, and then the wiring into hydration with the e2e rows and the docs.

A user sees one change: each cache repository directory gets an empty file codestorage.gitlease.lock (mode 0600) next to codestorage.hydrate.lock. Nothing waits on it yet. On a read-only filesystem, reads still work as on main (see below). Three cases make every git start in a repository fail with an error where main did not, and each needs an operator or a full disk. (a) The lease file exists but serve cannot open it for writing, after a manual chmod or chown. (b) The repository root is not writable and has no lease file. (c) The disk is full and the repository has no lease file yet, which only a repository without a git start since the upgrade can be in. In each case the error names the lease file.

What changes

  • gitlimit.StartIn, RunIn, OutputIn and CombinedOutputIn start a git child with a shared flock on <repo>/codestorage.gitlease.lock as its descriptor 10 (gitlimit.LeaseFD). Git reads the descriptors 2 to 9 as trace targets (GIT_TRACE=3), so the child has descriptors 3 to 9 closed. The parent closes its own copy after Start. The child, its hooks and their children keep the descriptor. So the lock lasts as long as the process tree, and an orphan of a crashed serve still holds it. TryExclusive takes the lock exclusively without waiting. Success proves that no git that serve started is alive in that repository. A start that finds the lock held exclusively waits up to 30 s and then fails with ErrLeaseBusy. The wait ends at once with the caller's context error when that context ends. Nothing takes the exclusive lock in this PR.
  • Every git child that runs in a cache repository moves to that API. These are gitread.Run, the advertise, upload-pack and receive-pack children in githttp/server.go, the commit-graph write, hydration's git init, for-each-ref stream and update-ref --stdin, and the REST write helpers in internal/api. The repack functions runGitStdin and streamShowIndex and the LFS scan start git themselves. They call gitlimit.OpenLease and Lease.Attach, and their signatures are the same. Two calls stay plain because they read no repository: git check-ref-format and git version.
  • Hooks need no change. A hook runs inside a git that holds the lease and passes descriptor 10 on to its own git children.
  • Two guards cover a missed call site. That is the one risk of this design, because a cleanup that removed the lock of a live git would lose a ref update without any error.
    • TestEveryGitStartTakesTheLease parses every non-test Go file in the module, in two layers. A file that imports os/exec must be in execImporters with a reason, so a new file cannot start git unseen. Inside one function (or one package-level var), the audit fails on a plain gitlimit.Start, Run, Output or CombinedOutput, on a function that builds git and starts none through a lease-aware starter, and on a direct cmd.Start, Run, Output or CombinedOutput in such a function. It sees a command built with exec.Command, exec.CommandContext, an exec.Cmd literal, exec.Command held as a value, or a shell or launcher program such as sh -c. TestAuditSeesEveryWayToBuildGit proves each of those on small sources. Each allow-list entry has a reason. The audit does not see a helper in another package that starts git for its caller, or git started without os/exec, and the test says so.
    • A binary built with the openstorage_viewcheck tag panics when a plain start runs in a repository, or when a leased start runs in another repository than its lease. CI's test build and the e2e serve binary use that tag. Release builds do not.
  • On a read-only filesystem (EROFS), OpenLease does not fail: nothing can write a ref lock there, and reads work on main. An existing lease file is opened read-only and locked (flock needs no write access), so the lease is a real one. With no lease file, the lease holds nothing and Attach adds no descriptor. Any other failure is an error, a permission error included: a directory that serve cannot write can still have a writable refs/, where a git without the lease could take ref locks.
  • A start that fails before the child runs (the lease wait ended, or StopChildren began) closes the pipes that the caller made with StdinPipe and StdoutPipe. The commit-graph attempt passes its own context, so its budget bounds the wait for the lease, and ReachableLFSOIDs takes the lease before it makes its pipes.
  • Docs: internal/gitlimit/README.md (interface, invariants, side effects) and one line each in the lfsscan and jobs READMEs.
  • The lane-schedule model is recorded again with check.sh --record. The covered functions that start git changed only in the starter they call, and the model describes push-lane scheduling, not git starts.

Verification

  • TestLeaseLivesWithTheGitProcessTree runs a real git update-ref whose reference-transaction hook parks in the prepared state, directly and behind the heap-cap shell wrapper. The exclusive try stays busy after the parent closed its copy and after only git (the leader) died. It succeeds once the whole process group is killed, and the stale refs/heads/x.lock stays. That is the githttp: a stale ref lock left by a killed receive-pack can wedge fetches on that node #464 shape. TestStartInWaitsForAnExclusiveHolder, TestStartInStopsWaitingWhenItsContextEnds, TestTheLeaseIsOnlyOnLeaseFD, TestGitTraceToADescriptorBelowTenNeverReachesTheLease, TestLeaseEndsWithTheChildAndTheFileStays, TestFailedStartReleasesTheLease and two openstorage_viewcheck tests cover the rest of the API and the panics.
  • Mutation checks, each against the test that names it:
    • StartIn without Attach: TestLeaseLivesWithTheGitProcessTree/direct and /heap_cap fail with TryExclusive while the tree lives = <nil>, want ErrLeaseBusy.
    • StartIn that keeps the parent's copy: the same test fails with timed out waiting for the lease to end with the tree, and TestLeaseEndsWithTheChildAndTheFileStays and TestFailedStartReleasesTheLease fail with git lease is busy.
    • The context check removed from OpenLease: TestStartInStopsWaitingWhenItsContextEnds fails in both cases with RunIn kept waiting for the lease after its context ended.
    • LeaseFD set back to 3: TestTheLeaseIsOnlyOnLeaseFD fails with exit status 3, and TestGitTraceToADescriptorBelowTenNeverReachesTheLease fails with git wrote 81 bytes of trace into the lease file. Its control shows that git writes that trace into a lease on descriptor 3.
    • A lease dropped at one call site, three ways: updateRefStdin back to plain CombinedOutput, gitread.Run back to plain Run, and wrefGit back to cmd.Output(). TestEveryGitStartTakesTheLease fails on each and names the line. With gitread.Run plain, go test -tags=openstorage_viewcheck ./internal/gitread also panics: starts in ... without a lease; use gitlimit.StartIn.
    • The viewcheck build caught a real case during this work. It refused the unleased git init --bare of a half-initialized repository in TestEnsureCacheRepoCtx_PartialInitialization. The init now takes the lease.
  • Round 3 (the root's verifier found two blocking gaps at 2588739f, and these fixes answer it). New unit tests, each with a mutant that turns it red:
    • TestRunGitStdinHoldsTheLeaseInTheChildAndReleasesItsOwn, TestStreamShowIndexHoldsTheLeaseInTheChildAndReleasesItsOwn and TestReachableLFSOIDsLeaseFollowsEveryStage put a fake git first on PATH that exits and leaves a sleeper child behind. After the call returned, the exclusive try is busy, because the sleeper holds the lease. It succeeds once the sleeper is killed, so the parent kept no copy of its own. The LFS scan leaves three sleepers, one per stage, and the test kills them one by one and checks the lease stays busy until the last one is gone. Red mutants: no defer lease.Close() in runGitStdin, streamShowIndex and ReachableLFSOIDs, and Attach for the first stage only or for all but the last stage.
    • TestSharedLeasesCoexist (mutant: LOCK_EX in OpenLease), TestLeaseFileIsPrivate (mode 0644), TestOpenLeaseStopsWaitingWhenChildrenAreStopping (the Stopping() check removed), TestOpenLeaseGivesUpAfterTheLeaseWait (the 30 s bound removed), TestAttachRefusesAnExclusiveLease (the assertf removed) and TestReadOnlyFilesystemWithALeaseFileHoldsARealLease, TestReadOnlyFilesystemWithoutALeaseFileStillRuns, TestReadOnlyFilesystemWithAnUnreadableLeaseFileFails and TestOpenLeaseFailsClosedOutsideAReadOnlyFilesystem (the read-only boundary: five mutants of the fallback, and a permission error that falls back, are red). On a real read-only ext4 mount, a child ran without descriptor 10 and TryExclusive failed closed.
    • The audit has a mutant for each detector (exec.Cmd literal, exec.Command as a value, a program that is not in nonGitPrograms, os.StartProcess and syscall.Exec, a dot import, package-level var, import alias). TestAuditChecksTheAllowLists pins the stale-entry and unlisted-importer checks on synthetic input. The audit no longer relies on a list of launchers, because any program named by a literal that is not gpg or ssh-keygen counts as git.
    • TestReachableLFSOIDsLeaseFollowsEveryStage checks that the child of each stage holds the lease file as descriptor 10, so a stage that misses the lease fails whatever order they exit in. TestStartInClosesThePipesOfAStartThatFails, TestCommitGraphAttemptBudgetBoundsTheLeaseWait and TestReachableLFSOIDsLeavesNoPipeOpenWhenTheLeaseFails have red mutants as well.
  • Lanes at the round-4 head 3ffbca1e, with -tags=openstorage_viewcheck -race -count=1: ./internal/githttp with OPENSTORAGE_TEST_BACKEND=local ok (1205.414 s), ./internal/jobs ok (531.480 s), ./internal/api ok (548.429 s), ./cmd/openstorage ok (91.838 s). The s3wal run of ./internal/githttp failed three tests, TestPush_PackPathJITFanoutSurvivesSmallBudget, TestPush_DoomedCreationReleasesHoldOnDenial and TestAtomicPush_StaleBaseDeniedBeforeAnyCAS, each at start node with listen tcp 127.0.0.1:0: bind: address already in use. That is loopback port exhaustion on the shared host (about 12,500 sockets in TIME_WAIT), before any code under test ran, so those three count as not run. I ran the three again alone with the same flags on the same head: all pass (57.177 s). The CI job test-s3wal passed on the head.
  • Lanes at the merged head a452f796 (main's fix(api): answer 404 or 400 for a git check only when git says no #760 and fix(sync): refuse stale held snapshots on PostgreSQL and restore River push runs #762 merged in, before the fixes of review round 1), with -tags=openstorage_viewcheck -race -count=1: ./internal/jobs ok (524.060 s), ./internal/api ok (576.693 s), ./cmd/openstorage ok (86.486 s), ./internal/githttp with OPENSTORAGE_TEST_BACKEND=local ok (863.674 s) and with OPENSTORAGE_TEST_BACKEND=s3wal ok (1324.663 s).
  • Lanes at the earlier head 2588739f, with -tags=openstorage_viewcheck -race -count=1: ./internal/jobs ok (372.379 s), ./internal/api ok (325.684 s), ./cmd/openstorage ok (74.322 s), ./internal/githttp with OPENSTORAGE_TEST_BACKEND=local ok (909.097 s) and with OPENSTORAGE_TEST_BACKEND=s3wal ok (1218.973 s).
  • Codex: four review requests, four of six rounds. Round 1 (a452f796) raised two P2 findings (the lease wait ignored the caller's context, and descriptor 3 is one of Git's trace descriptors), both fixed in 04caacac. Round 2 (2588739f): the code review and the security review found nothing. Round 3 (2238f844) raised three P2 findings in the code review (a read-only lease file, the commit-graph attempt's context, and pipes left open when the lease start fails), all fixed and resolved. Round 4 on the final head 3ffbca1e: the code review found no major issues, and the security review found no security issues. All five review threads are resolved.
  • CI on the final head 3ffbca1e: lint, test, test-local, test-s3wal, pr-title, terraform, both image jobs and both fingerprint jobs (lane-schedule-fingerprints, orphan-sweep-fingerprints) passed. Main's fix(gitread): decline a tree walk whose cat-file ended before the request #774 touches only internal/gitread/tree_resume.go, which the audit already lists.
  • An earlier run of the local lane, at 403912fe, failed after 884 s. My script kept only the last 40 lines, so I did not record which test failed and I cannot name it. Two later runs of that head passed (817.301 s and 787.329 s). Two more runs that I started in parallel failed in 19 tests with bind: address already in use on 127.0.0.1:0, which is loopback port exhaustion on the shared host, so they are not a result.
  • ./internal/gitlimit, ./internal/gitread, ./internal/lfsscan and ./internal/simtest with the same flags: ok.
  • On a real openstorage serve (verify-openstorage, osv.sh launch): after one push and one clone the cache repository holds the 0-byte codestorage.gitlease.lock. flock -xn on it succeeded while the node was idle. It failed ("busy") while a 400 MB clone's client was stopped (SIGSTOP on the clone's own descendants), so the upload-pack was blocked on its output. It succeeded again when the clone had ended.
  • go build ./... and go vet -tags=openstorage_viewcheck on the touched packages: clean. golangci-lint 2.15.0 on the touched packages: 0 issues. go build ./... and go test -c for internal/gitlimit on darwin and linux, amd64 and arm64: ok.
  • .agents/skills/verify-lane-schedule/check.sh --record: every property holds and the probe fails. Both check.sh --fingerprints-only gates (verify-lane-schedule, verify-orphan-sweep): PASS.
  • python3 scripts/check-doc-links.py: 0 broken links. python3 scripts/test_check_doc_links.py: 80 tests OK.

Refs #490


Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh

… repository

Every git that serve starts in a cache repository now inherits a shared
flock on <repo>/codestorage.gitlease.lock as its descriptor 3. The lock
lives as long as the git process tree, so a later exclusive try proves
that no git serve started is alive in that repository. Nothing takes the
exclusive lock yet (#490).

The call sites that run git in a cache repository move to
gitlimit.StartIn, RunIn, OutputIn and CombinedOutputIn, or attach a lease
themselves (repack, lfsscan). TestEveryGitStartTakesTheLease fails on a
git start without a lease that is not listed with a reason, and a binary
built with openstorage_viewcheck panics on one that runs in a repository.

Signed-off-by: Thomas Kosiewski <tk@coder.com>

---
_Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_

Change-Id: I0fe3fd8bf9a454177b2578fa824fa5a85cbd2a98
TestEnsureCacheRepoCtx_PartialInitialization re-inits a repository that
holds HEAD and objects, so the viewcheck build refused the unleased
`git init --bare`. EnsureCacheRepoCtx creates the directory before it
runs git, so the init takes the lease like every other git in the
repository, and the audit allow-list loses its runGitCtx entry. The lease
test also runs git behind the heap-cap shell wrapper, and the viewcheck
panic text names the command once.

Signed-off-by: Thomas Kosiewski <tk@coder.com>

---
_Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_

Change-Id: I0c7173de74fe72e4d30dd118beb3e80cb3a37e3b
…tarters

The covered functions that start git changed only in the starter they
call (StartIn, RunIn, OutputIn, CombinedOutputIn) and in the new
runGitLeased helper: none of them changes push-lane scheduling, the
ordering of a hold, a claim or a publication, which the model describes.
check.sh --record passed with every property true and the probe failing.

Signed-off-by: Thomas Kosiewski <tk@coder.com>

---
_Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_

Change-Id: Ifbd9576c03e2cb8b507066286b74aa7bbe53a42f
Change-Id: I7b806b92e8d4551bc44623822a35db0c5199c452
Signed-off-by: Thomas Kosiewski <tk@coder.com>
Main landed #760 and #762, which touch the covered hydrate.go and
server.go. The merge result keeps the lease changes and the model
describes it: check.sh --record passed with every property true and the
probe failing.

Signed-off-by: Thomas Kosiewski <tk@coder.com>

---
_Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_

Change-Id: If01a9de30157fffae713a53a8a921c4a2559b51c
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 7, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-08T00:14:27.169819Z 3ffbca1 Manual request
🔒 Security Review ✅ Completed 2026-11-08T00:14:38.081193Z 3ffbca1 Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: a452f79647

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a452f79647

ℹ️ 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".

Comment thread internal/gitlimit/lease.go Resolved
Comment thread internal/gitlimit/lease.go Resolved
…git's trace descriptors free

Codex round 1 on #768 raised two findings.

The wait for an exclusive holder ignored the caller's context, so a
canceled request or an expired budget kept polling for up to 30 s before
git started. OpenLease, StartIn, RunIn, OutputIn and CombinedOutputIn
take the context first. The wait ends with the context's error as soon as
the context ends. updateRefStdin has no context and passes Background.

Git reads GIT_TRACE=3 to 9 as open trace descriptors, so a lease on
descriptor 3 received git's trace when an operator set GIT_TRACE=3. The
lease now sits on descriptor 10 (LeaseFD) and descriptors 3 to 9 stay
closed in the child.

Signed-off-by: Thomas Kosiewski <tk@coder.com>

---
_Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_

Change-Id: Iab13f1ee31fcd1b4299de5062674ef9dae318b06
…change

The covered functions that start git now pass their context to the lease
starters. None of them changes push-lane scheduling. check.sh --record
passed with every property true and the probe failing.

Signed-off-by: Thomas Kosiewski <tk@coder.com>

---
_Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_

Change-Id: Ie53be30080c1f0c0c60c3eff1fe5fd8fbec1c277
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: 2588739f98

ℹ️ 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".

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: 2588739f98

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

Change-Id: Ib914af347503e7a4dcc3720b7c7c471fddb63778
Signed-off-by: Thomas Kosiewski <tk@coder.com>
OpenLease created the lease file with O_CREATE, so a cache repository
directory that serve cannot write, and that has no lease file yet, made
every git read fail with an internal error. Main answers those reads. The
lease now holds nothing in that one case: no git of serve can take a ref
lock in a directory that git cannot write either.

The unit tests now cover what only the end-to-end tests caught before:
shared leases coexist, the file is private (mode 0600 under umask 0), a
start gives up after the lease wait, OpenLease stops waiting once the
children are stopping, and Attach refuses an exclusive lease.

Signed-off-by: Thomas Kosiewski <tk@coder.com>

---
_Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_

Change-Id: I62791b175573e0bafcaaf41466b4def9f979979a
… the audit

The audit test now has two layers. A non-test file that imports os/exec
must be in execImporters, so a new file cannot start git unseen. The
function-level check also sees an exec.Cmd literal, exec.Command held as
a value, a shell or launcher program, and a package-level initializer.
TestAuditSeesEveryWayToBuildGit proves each of them on small sources.

runGitStdin, streamShowIndex and ReachableLFSOIDs get tests that a fake
git holds the lease while it runs, and that the parent's copy is closed
when the call returns. For ReachableLFSOIDs the test checks all three
stages. The README of gitlimit states the viewcheck invariant as the
code has it, and the comments no longer name descriptor 3.

Signed-off-by: Thomas Kosiewski <tk@coder.com>

---
_Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_

Change-Id: Iaaba5c3cb166c1217fc6525c8564fc37681c3520
check.sh --record passed: every property holds, LaneSchedule, OtherWriters
and Liveness pass, and the probe fails. The fingerprints are unchanged.

Signed-off-by: Thomas Kosiewski <tk@coder.com>

---
_Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_

Change-Id: I32fc087261cb03cca31f4a6cf43e9b4d6ed87f47
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: 2238f844fd

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2238f844fd

ℹ️ 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".

Comment thread internal/gitlimit/lease.go Resolved
Comment thread internal/githttp/commitgraph.go Resolved
Comment thread internal/gitlimit/lease.go Resolved
… the pipes of a start that fails

OpenLease fell back to a lease that holds nothing for any permission
error when the directory had no lease file. A directory that serve cannot
write can still have a writable refs/, so a git that ran there took ref
locks with no lease, and a later TryExclusive could win while it lived.
Now only EROFS selects the fallback. An existing lease file is opened
read-only and locked, because flock needs no write access, so the lease is
a real one. With no lease file the lease holds nothing, because nothing can
write on a read-only filesystem. A permission error is an error.

A start that fails before cmd.Start runs (the lease wait ended, or
StopChildren began) now closes the pipes that the caller made with
StdinPipe and StdoutPipe: exec closes them only in a Start that runs.

Unit tests pin the boundary with a seam that reports EROFS (a missing
directory, a directory serve cannot write, a lease file nobody can open,
and an unreadable lease file on a read-only filesystem all fail), the real
lease on a read-only filesystem, the pipes, and the Attach assertion on
ExtraFiles.

Signed-off-by: Thomas Kosiewski <tk@coder.com>

---
_Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_

Change-Id: I2dae79c5a6e84b2b3b741ae5e1abd1a872b90f79
…pen the scan's lease before its pipes

The commit-graph attempt passed the outer context to the lease start, so a
write that queued behind an exclusive holder kept waiting after its budget
ended. It now passes the attempt's context.

ReachableLFSOIDs made the pipes of its three stages before it took the
lease, so a refused scan left ten descriptors open until a garbage
collection. It takes the lease first. The per-stage test now checks that
the child of every stage holds the lease file as descriptor 10, whatever
order they exit in.

Signed-off-by: Thomas Kosiewski <tk@coder.com>

---
_Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_

Change-Id: I63eafb01fc9497e20109e5301af2058df7219bd2
…d process-start blind spots

The audit checks of a stale allow-list entry and of an unlisted file that
imports os/exec had no test, because the module holds neither today.
TestAuditChecksTheAllowLists runs them on synthetic input.

A program named by a string literal now counts as git unless it is in
nonGitPrograms (gpg, ssh-keygen), because no list of launchers (sh, env,
flock, taskset, ...) is complete. os.StartProcess and syscall.Exec count
as building a process, and a dot import of os/exec or gitlimit fails. The
README states the read-only rule and the audit as built.

Signed-off-by: Thomas Kosiewski <tk@coder.com>

---
_Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_

Change-Id: If7b0ff975b0fb3707d8f2e653beeb483b9a5750b
Change-Id: I457ad1be11c5da01d355f1e1a46b365cfce8fba8
Signed-off-by: Thomas Kosiewski <tk@coder.com>
check.sh --record passed: every property holds, LaneSchedule, OtherWriters
and Liveness pass, and the probe fails. The fingerprints are unchanged.

Signed-off-by: Thomas Kosiewski <tk@coder.com>

---
_Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_

Change-Id: I52089c622a3b9546840b11627d794c4dfb69bf42
@ThomasK33

Copy link
Copy Markdown
Member Author

@codex review

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

Reviewed commit: 3ffbca1eee

ℹ️ 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".

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: 3ffbca1eee

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

ThomasK33 added a commit that referenced this pull request Nov 8, 2026
… lease epoch only when serve creates the repository

The merge brings the lease head 2238f84 (round 3 of #768) into this
branch. The conflict was in gitlimit/lease.go, resolved with the lease
side plus this branch's epoch functions.

The epoch fix answers a Codex P1 on this PR: a git of an older release,
which holds no lease, can outlive its server (a kill, and a graceful stop
of v0.18.0, which exits 0 with its git children running). If OpenLease
wrote the epoch at the first lease-aware git start in such a repository,
a lock that this git made after that start was newer than the epoch, and
the cleanup would delete it.

Now only CreateEpoch writes the epoch, and EnsureCacheRepoCtx calls it
when its own os.Mkdir created the repository directory, before git runs
there. OpenLease and TryExclusive write nothing. A repository without an
epoch (an older release created it, or its creation was interrupted) keeps
every lock: cleanStaleRefLocks counts kept, logs one runbook line, takes
no lease and writes no epoch.

Signed-off-by: Thomas Kosiewski <tk@coder.com>

---
_Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_

Change-Id: Ie52ced9dff595fb2084ff4ddfd32fc3afc908994
@ThomasK33
ThomasK33 added this pull request to the merge queue Nov 8, 2026
Merged via the queue into main with commit f3fc471 Nov 8, 2026
13 checks passed
@ThomasK33
ThomasK33 deleted the issue490-lease branch November 8, 2026 02:15
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.

1 participant