[codex major + omp major-13, same root] Repo-import detached from server cancellation #74

New issue

Closed

opened 2026-09-05 00:15:03 +00:00 by crueber · 3 comments

crueber commented

2026-09-05 00:15:03 +00:00

Owner

[codex major + omp major-13, same root] Repo-import detached from server cancellation

internal/repoimport/service.go:232 starts drive in a goroutine built from context.WithoutCancel(context.Background()) (:278). Shutdown/drain cannot cancel the import or its store work; the task survives until internal timeouts, and can even commit a manifest CAS after drain begins. Violates "every goroutine exits via context" and the 13 §8 phase-1-ctx rule (omp notes it matches the opsTasks precedent at serve.go:594 — either way it must be fixed or get an explicit doc decision; fix preferred: wire the phase-1/drain context through, keep WithoutCancel ONLY where join semantics require it, and forbid post-drain manifest commits).

Fix

Thread a cancellable context (drain-scoped) through the import leader; cancel git children + store work on drain; manifest commit refuses after drain begins. Regression test (start import, drain, assert prompt terminal error + no commit). Coverage gate holds; doc Decisions entry (law 12).

Acceptance criteria

Drain cancels an in-flight import promptly; no post-drain commit.
go test -race + coverage ≥95% on internal/repoimport.

# [codex major + omp major-13, same root] Repo-import detached from server cancellation `internal/repoimport/service.go:232` starts `drive` in a goroutine built from `context.WithoutCancel(context.Background())` (`:278`). Shutdown/drain cannot cancel the import or its store work; the task survives until internal timeouts, and can even commit a manifest CAS after drain begins. Violates "every goroutine exits via context" and the 13 §8 phase-1-ctx rule (omp notes it matches the `opsTasks` precedent at `serve.go:594` — either way it must be fixed or get an explicit doc decision; fix preferred: wire the phase-1/drain context through, keep `WithoutCancel` ONLY where join semantics require it, and forbid post-drain manifest commits). ## Fix Thread a cancellable context (drain-scoped) through the import leader; cancel git children + store work on drain; manifest commit refuses after drain begins. Regression test (start import, drain, assert prompt terminal error + no commit). Coverage gate holds; doc Decisions entry (law 12). ## Acceptance criteria - [ ] Drain cancels an in-flight import promptly; no post-drain commit. - [ ] `go test -race` + coverage ≥95% on `internal/repoimport`.

crueber referenced this issue from a commit

2026-09-05 00:49:46 +00:00

Fix #74: drain-scoped repo-import cancellation

crueber referenced this issue from a pull request that will close it,

2026-09-05 00:49:49 +00:00

Fix #74: drain-scoped import cancellation #83

crueber commented

2026-09-05 00:49:52 +00:00

Author

Owner

Fixed by PR #83 (#83): drain-scoped import cancellation — leader wait on the service drain ctx, Begin/commit-point 503 guards, table+service drain at phase 1, plus the latent TaskTable rt.cancel race fix. Not merging per instructions.

Fixed by PR #83 (https://git.packden.us/crueber/walhub/pulls/83): drain-scoped import cancellation — leader wait on the service drain ctx, Begin/commit-point 503 guards, table+service drain at phase 1, plus the latent TaskTable rt.cancel race fix. Not merging per instructions.

crueber commented

2026-09-05 00:53:31 +00:00

Author

Owner

PR #83 review (fix/issue-74, commit 66bc6d8) — verified in scratch worktree /tmp/wt74 (pre-existing; left in place). Main worktree untouched (read-only).

PASS

WithoutCancel removal complete in repoimport (only remaining mention is a doc comment, service.go:283). Join semantics preserved: leader derives from drainCtx, never the request ctx, so client disconnect still cannot cancel it; only Drain cancels. Other WithoutCancel uses (cmd/walhub/serve.go:606 opsTasks, pulls, notify, bundle, server/auth) are separate surfaces, out of scope.
Drain lifecycle: Service.Drain idempotent (mu-guarded flag + idempotent CancelFunc called outside the lock); double-Drain covered by TestBeginAfterDrain503. TaskTable.Drain idempotent (drainMu+mu early return).
Commit guard ordering (task.go:119-124): Draining() flag first, ctx.Err() second, both 503 before the manifest PutCreate CAS. Either suffices.
serve.go:250-262 drains BOTH halves table-first (reg.Tasks().Drain() then importSvc.Drain()), nil-guarded, placed after RunPhase1 / before maintainer join. Both Drains are non-blocking (flag+cancel only), so no deadlock and no added drain latency.
wal rt.cancel race fix (tasks.go:188-218): runCtx/cancel created before lock, published WITH the map entry; join path (192) and refuse path (204) call the unused cancel; completion goroutine calls cancel (255); Drain cancels are idempotent. Every created CancelFunc is stored-or-called: no context leak. -race green.
Regression tests genuine: pre-fix, the hanging-clone run yields 503 'clone interrupted...' (classifyCloneError) which fails the 'draining' message assertion; Begin-after-drain and commit-guard tests fail pre-fix (no refusal/guard existed). Mid-clone cancel exercised via hanging-git fixture (exec-replaced sleep, SIGKILL-reaped, no orphans).
Coverage: repoimport 95.6%, wal 95.3% (gate holds). No new imports (test file: stdlib + internal/store only). gofmt clean, go vet clean. Doc entry in 10_git_import.md matches the shipped code.

OBSERVATIONS (non-blocking, no fix pushed)

Residual TOCTOU: guard-to-CAS window (task.go:119 -> reg.Create). Drain landing inside it relies on runCtx cancellation failing Create's Put; the memory store ignores cancelled ctx at zero latency (memory.go tick only selects on ctx with Latency>0). S3/GCS/filesystem-permit paths honor ctx. True impossibility needs a drain-aware CAS; the current check-then-act + dual-drain ordering reduces the window to untestable size. Acceptable.
Begin probe window: Drain during probe installs a stillborn running entry; drive then fails fast 503 via cancelled drainCtx/table refusal — safe, no commit possible. Could re-check Draining() under lock before install to avoid the stillborn task; cosmetic.

TESTS (scratch worktree, -race -count=1): internal/repoimport ok (26.2s); internal/wal + wal/rw + cmd/walhub ok. Coverage run: 95.6%/95.3%. gofmt -l clean; vet clean.

MERGE RECOMMENDATION: ready to merge.

PR #83 review (fix/issue-74, commit 66bc6d8) — verified in scratch worktree /tmp/wt74 (pre-existing; left in place). Main worktree untouched (read-only). PASS - WithoutCancel removal complete in repoimport (only remaining mention is a doc comment, service.go:283). Join semantics preserved: leader derives from drainCtx, never the request ctx, so client disconnect still cannot cancel it; only Drain cancels. Other WithoutCancel uses (cmd/walhub/serve.go:606 opsTasks, pulls, notify, bundle, server/auth) are separate surfaces, out of scope. - Drain lifecycle: Service.Drain idempotent (mu-guarded flag + idempotent CancelFunc called outside the lock); double-Drain covered by TestBeginAfterDrain503. TaskTable.Drain idempotent (drainMu+mu early return). - Commit guard ordering (task.go:119-124): Draining() flag first, ctx.Err() second, both 503 before the manifest PutCreate CAS. Either suffices. - serve.go:250-262 drains BOTH halves table-first (reg.Tasks().Drain() then importSvc.Drain()), nil-guarded, placed after RunPhase1 / before maintainer join. Both Drains are non-blocking (flag+cancel only), so no deadlock and no added drain latency. - wal rt.cancel race fix (tasks.go:188-218): runCtx/cancel created before lock, published WITH the map entry; join path (192) and refuse path (204) call the unused cancel; completion goroutine calls cancel (255); Drain cancels are idempotent. Every created CancelFunc is stored-or-called: no context leak. -race green. - Regression tests genuine: pre-fix, the hanging-clone run yields 503 'clone interrupted...' (classifyCloneError) which fails the 'draining' message assertion; Begin-after-drain and commit-guard tests fail pre-fix (no refusal/guard existed). Mid-clone cancel exercised via hanging-git fixture (exec-replaced sleep, SIGKILL-reaped, no orphans). - Coverage: repoimport 95.6%, wal 95.3% (gate holds). No new imports (test file: stdlib + internal/store only). gofmt clean, go vet clean. Doc entry in 10_git_import.md matches the shipped code. OBSERVATIONS (non-blocking, no fix pushed) - Residual TOCTOU: guard-to-CAS window (task.go:119 -> reg.Create). Drain landing inside it relies on runCtx cancellation failing Create's Put; the memory store ignores cancelled ctx at zero latency (memory.go tick only selects on ctx with Latency>0). S3/GCS/filesystem-permit paths honor ctx. True impossibility needs a drain-aware CAS; the current check-then-act + dual-drain ordering reduces the window to untestable size. Acceptable. - Begin probe window: Drain during probe installs a stillborn running entry; drive then fails fast 503 via cancelled drainCtx/table refusal — safe, no commit possible. Could re-check Draining() under lock before install to avoid the stillborn task; cosmetic. TESTS (scratch worktree, -race -count=1): internal/repoimport ok (26.2s); internal/wal + wal/rw + cmd/walhub ok. Coverage run: 95.6%/95.3%. gofmt -l clean; vet clean. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-05 00:53:53 +00:00

crueber commented

2026-09-05 00:53:53 +00:00

Author

Owner

Fixed by PR #83 (review clean; drain cancels import, no post-drain commit; 95.6%/95.3% coverage), merged. Closing.

crueber referenced this issue from a commit

2026-09-05 00:53:53 +00:00

Merge pull request 'Fix #74: drain-scoped import cancellation' (#83) from fix/issue-74 into main

crueber referenced this issue

2026-09-05 20:39:02 +00:00