Repository navigation
fix(openshell): drop withdrawn git routes and report store closures - #3271
Conversation
A withdrawn route stayed in the worker's maps for its whole life, and the 1 s tick and unmatched lookups walked every one. A closed route whose Sandbox is gone is now dropped to its reason: the revision stays refused in this process, and the store's tombstone refuses it elsewhere. The route store's owns() answered false both for a moved fence and for a row closed at the holder's fence, so a route another worker withdrew or expired was dropped as worker_replaced and the Agent got a generic denial. owns() now answers the closed row's reason, the write and record refusals ask for it, and the route closes with access_withdrawn. A withdrawal the store does not record is logged instead of swallowed. Finding 2218.
…store closures Review follow-ups: an entry closed before its Sandbox was observed is dropped after two absent reads; Sandbox reads back off from 10 s to 60 s while it stays; store-reported closures travel as their own error class instead of being inferred from the reason; tests pin the verifyExecution guard, the read throttle, closure during a pending read and the never-observed case.
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
clawsweeper:intake:bug
|
Codex review: needs maintainer review before merge. What this changesThis PR releases closed OpenShell Git route entries after Sandbox removal, preserves store closure reasons, and reports failed withdrawal writes. Example: Another worker withdraws revision rev-one while its Sandbox remains running.
Review scores
ProductKind: Bug fix · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep this PR open: it repairs concrete defects still present on main, resolves the prior documentation finding, and introduces no actionable defect found in this review. Priority: P2 Before mergeNone. FindingsNone. Tests
Agent review detailsHow this fits togetherThe controller’s OpenShell Git runtime combines revision and Sandbox observations with PostgreSQL route fences to supply request bindings, readiness states, and denial diagnostics. flowchart TD
A[Sandbox lifecycle] --> B[OpenShell Git runtime]
C[Agent Git request] --> D[Online Git authority]
D --> B
B --> E[PostgreSQL route store]
B --> F[OpenShell Gateway]
B --> G[Blocked revision record]
Technical reviewBest possible solution: Retain this focused runtime-and-store repair with its blocked revision records and updated lifecycle documentation. Do we have a high-confidence way to reproduce the issue? Current-main source directly establishes retained withdrawn entries and misclassification of same-fence closures; no target code was executed during this read-only review. Is this the best way to solve the issue? The patch uses the existing runtime and persistence owners, preserves fence enforcement, and prevents releasing heavyweight entries from reopening the revision. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against fcebb0939c09. Provenance checked
TestingProof path: in-process harness. SecurityNone. EvidenceWhat I checked:
Review metrics
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (1 earlier review cycle)
Reviewed October 23, 2026, 12:44 AM ET / 04:44 UTC (Revision 2). |
The never-observed release path read entry.binding before the guarded call, so an entry without one (a synthetic preview entry in the standing-route test) threw out of reconcile. Any failure building or answering the read now keeps the entry, as a failed read already did.
… route reference The installation-custody flow names release(); the reference gives its conditions (absent Sandbox, twice if never observed; a failed read keeps the route) and the refusal that stays after the drop.
|
@clawsweeper re-review |
|
🦞👀 Re-review progress:
|
clawsweeper
Bot
added
clawsweeper:intake:bug
Problem
Two bookkeeping gaps in the OpenShell Git runtime (
apps/controller/src/drivers/egress-proxy/openshell-git-runtime.ts):withdraw()/withdrawNamespace()kept every withdrawn route's entry (the full AgentRevision, provider and binding) for the life of the worker. The 1 s reconcile tick and every unmatched lookup walked them all.owns()answeredfalseboth when another worker took the fence and when the row was closed at the holder's own fence. Withdrawal and expiry close the row without moving the fence, so a route that another worker withdrew or expired was logged asworker_replacedand dropped. Its Agent then got a generic denial instead ofaccess_withdrawn.withdraw()also wrote the store fire-and-forget with an emptycatch.Change
OpenShellGitRouteStore.owns()returnstrue,false(fence moved or no row), or the closed row's reason when the row is closed at the holder's fence. The Postgres store and the in-memory test store implement the same contract.fenced()andpersist()turn a refusal into the store's reason. After a refused write reservation or record they askowns()why. A store-reported closure travels as its own error class.block()keeps the entry for it (so lookups answeraccess_withdrawn), writes nothing, and logs aclosedroute warning rather thanOPENSHELL_GIT_RECONCILIATION_REQUIRED. A moved fence is stillworker_replaced, as before.{ blocked: reason }state. A Sandbox the route never observed may still be in creation, so it must read absent twice, a refresh interval apart. That state keepsprepare(),verifyExecution()androute()refusing the revision in this process, without asking the store. The store's tombstone refuses it to any other worker. Waiting for the Sandbox to go, rather than dropping atwithdraw(), keeps theaccess_withdrawnanswer while the Sandbox can still ask. It also releases routes that another worker withdrew, whose cleanup runs elsewhere.worker.errorOPENSHELL_GIT_WITHDRAWAL_UNRECORDED, with the selector and error class.closedreasons and the new code, and gives the release conditions. The egress flow's installation-custody step namesrelease(that page stays at 2,494 of its 2,500 words).Verification
tests/conformance/openshell-git-runtime.test.mjs(both fail on main):withdrawnandrepository_expired, the closure is seen by a lookup, by a reconcile pass and by a refused rotation reservation. In each case the Agent getsaccess_withdrawn, there is noworker.error, and nothing is written. An adopted fence is stillworker_replaced.prepare()is refused without any store call (no re-adoption, no re-bootstrap),verifyExecution()answers nothing, and a restarted worker hits the store tombstone. A store that misses the withdrawal is logged. The revision is still refused after the drop, and a standing execution is not verified from the store's unclosed row. A route withdrawn before its Sandbox was observed needs two absent reads. A read that answers afterclose()puts nothing back. Namespace withdrawal behaves the same.verifyExecutionguard, no identity re-check after the read, no throttle, and store closures inferred from the reason.access_withdrawninstead ofno_binding.tests/integration/postgres-openshell-git-routes.test.tsnow pinsowns(): it answerssupervisor_replaced,withdrawnandrepository_expiredat the holder's fence, andfalsefor a stale fence.openshell-git-runtimeconformance: 41/41 passed.repository-runtime-material,kubernetes-compute,repository-credentials-admission,protected-driver-registrationandsandbox-driver-startup: all passed.postgres-openshell-git-routes(8/8),postgres-openshell-standing-route(6/6),postgres-openshell-standing-git-transfer(1/1),protected-driver-registration(5/5) andrepository-credentials-admission(35/35) all passed.postgres-protected-git-recipient-custody(3/3) passed on the first commit.postgres-openshell-standing-route: its synthetic preview entry has no binding, and the release read built its request outside the guarded call. The read is now built inside it, so any failure keeps the entry.tsc --build(repo + tests), ESLint and Prettier on the touched files: clean.verifyExecutionguard, release a route closed before its Sandbox was observed, back off the reads, and mark store closures by error class.postgres-openshell-git-routespasses 8/8.tests/conformance/openshell-git-runtime.test.mjspassed 20/20 runs at 2 CPUs each, on the final head.Risks
GetSandboxread per closed route, every 10 s backing off to 60 s, until its Sandbox is gone. A withdrawn route usually loses its Sandbox within seconds (cleanup deletes it right afterwithdraw()). A route closed for another reason, or one retained after Git expiry, keeps polling at 60 s until its revision is cleaned up.{ blocked }record per closed revision stays for the life of the worker. That keeps a missed store write from letting this process re-prepare a withdrawn revision.access_withdrawnto generic. The route stays closed either way.