[minor-15] Star gate: process-global lock held across store calls #97

New issue

Closed

opened 2026-09-05 02:40:27 +00:00 by crueber · 3 comments

crueber commented

2026-09-05 02:40:27 +00:00

Owner

[minor-15] Star gate: process-global lock held across store calls

internal/social/social.go:150-157, service.go:44-45 (from PR #70): lockStar serializes ALL Stars instance-wide with getJSON/putCreate/CAS loops inside — a slow store call stalls every star on the instance. It is a new process-level lock outside the 13 §2 closed list (documented as an exception, and our reviewer passed it), but the finding stands: either refactor to pure-CAS (fold the resync into the bump's CAS loop so no gate is needed — the reviewer of #70 claimed this cannot close the creator-vs-observer hole; prove or disprove with a test), or keep the gate and tighten it (per-repo/per-target sharding? bounded hold with timeout?).

Fix

Either pure-CAS (preferred if it closes the hole — demonstrate with the 8-way stress test) or a tightened gate (sharded, documented, with a hold-time bound). Whichever: keep the #69 regression tests green, coverage ≥95%, doc Decisions entry updated honestly (law 12).

Acceptance criteria

No unbounded instance-wide serialization across store calls, or a justified documented exception with bounds; stress tests green -race -count=20.

# [minor-15] Star gate: process-global lock held across store calls `internal/social/social.go:150-157`, `service.go:44-45` (from PR #70): `lockStar` serializes ALL Stars instance-wide with `getJSON`/`putCreate`/CAS loops inside — a slow store call stalls every star on the instance. It is a new process-level lock outside the 13 §2 closed list (documented as an exception, and our reviewer passed it), but the finding stands: either refactor to pure-CAS (fold the resync into the bump's CAS loop so no gate is needed — the reviewer of #70 claimed this cannot close the creator-vs-observer hole; prove or disprove with a test), or keep the gate and tighten it (per-repo/per-target sharding? bounded hold with timeout?). ## Fix Either pure-CAS (preferred if it closes the hole — demonstrate with the 8-way stress test) or a tightened gate (sharded, documented, with a hold-time bound). Whichever: keep the #69 regression tests green, coverage ≥95%, doc Decisions entry updated honestly (law 12). ## Acceptance criteria - [ ] No unbounded instance-wide serialization across store calls, or a justified documented exception with bounds; stress tests green `-race -count=20`.

crueber referenced this issue from a commit

2026-09-05 03:28:22 +00:00

Fix #97: shard star gate per (repo, principal) (07 §4 resync kept)

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

2026-09-05 03:28:25 +00:00

Fix #97: star gate lock-across-store #106

crueber commented

2026-09-05 03:28:29 +00:00

Author

Owner

PR #106 (fix/issue-97) takes route (b): shards the star gate into 64 FNV-1a stripes keyed by (repo, principal) with a documented hold bound, instead of the process-global channel gate. Pure CAS was rejected with justification in the 07 Decisions entry. #69 stress tests green -race -count=20, new cross-repo contention test added, coverage 99.5%. Ready for review — not merging.

crueber commented

2026-09-05 03:31:08 +00:00

Author

Owner

PR #106 review (branch fix/issue-97, commit 5d8f7e4) — verified in scratch worktree, main worktree untouched.

FINDINGS (all pass, no fixes pushed):

Route-(a) rejection sound — internal/social/service.go:33-38. Create-path bump must stay unconditional: TestStarUnstarIdempotent pins second-principal → 2 on a nonzero counter, so conditioning the bump on absent/zero undercounts. A late observer whose initial GET sees the winner's just-created record cannot separate it from a stale record with the store CAS alone (record starred_at is RFC3339-seconds wall clock — comparing it to call-start is fragile, and versions are opaque). A CAS-only close would need a protocol change (e.g. record generation epoch) — disproportionate for human-rate Stars. Shard stays; reasoning accepted.
Stripe hashing sane — internal/social/social.go:138,146-162. FNV-1a (stdlib) over owner/repo+\x00+principal; fixed [64]sync.Mutex, no map growth/GC; deterministic so same (repo,principal) always shares a shard. Collision = unrelated Stars share a stripe: bounded (~1/64 of Star-only, human-rate traffic), acceptable.
Zero-Service path preserved — social.go:179-182 nil-shards no-op; TestStarZeroServiceGateSkips green -race -count=10.
Never-nested — single lockStar call site (service.go:50); Unstar takes no gate (version-conditional Delete only), reads lock-free. No path acquires two stripes or re-enters (grep-confirmed).
Hold bound honest — lock acquired AFTER requireRead+repoAlive (service.go:40-51, both outside the hold); inside: record GET + record PUT + one ≤8-attempt counter CAS loop (casUpdate attempts=8: social.go:258; reconcileStar:91, bumpStars:161). Reconcile/412 paths hold strictly less. All small control-plane JSON, no LIST/git/bulk. Doc claim '≤1 GET + 1 PUT + ≤8 CAS' verified against all three Star branches incl. resync.
Slow-store test genuine AND dispositive — service_test.go:187-238. stallStore wedges Get/Put/Delete but not Head, and repoAlive uses Exists→Head (store.go:231), so the wedge fires at the record GET inside the shard, exactly as the test comment claims. Empirical proof: with lockStar emulated as a process-global mutex the new test FAILS ('unrelated Star stalled', 10.0s); with the shipped shards it PASSES -race -count=10. Same-principal/two-repos/different-shards (collision avoided by construction, :190-199); both counters converge to 1.
#69 stress green for the right reasons — TestStarConcurrentResyncConverges (8x same jane → one shard → serializes, converges to 1, both absent/zeroed) and TestStarConcurrentConverge (jane+u → two shards → CAS convergence to 2) green -race -count=10. Cross-principal same-repo correctness still rests on CAS, as documented.
Hygiene — coverage 99.5% (≥95% gate holds); gofmt clean; go vet clean; no new non-stdlib imports (hash/fnv + sync only, go.mod untouched); full package suite green -race.
Docs accurate — 07 §4 rewrite + Decisions entry match the code (shard key, hold bound, regression test names); bound also documented at the lockStar site.
13_concurrency.md untouched — ACCEPTABLE, no amendment required. The gate is feature-level (Service shards), not a RepoHandle lock, so it does not breach the §2 closed repo-lock list; the lock-across-store pattern predates #97 (#69 global gate already in main) and #97 strictly shrinks the hold domain (global → ~1/64 shard) with a documented bound. Non-blocking recommendation: follow-up issue to codify feature-level gate rules in 13 §2 (the 'complete list' sentence is now visibly incomplete) so the next gate does not relitigate this.

MERGE RECOMMENDATION: ready to merge. No changes pushed; scratch worktree /tmp/pr106 removed.

PR #106 review (branch fix/issue-97, commit 5d8f7e4) — verified in scratch worktree, main worktree untouched. FINDINGS (all pass, no fixes pushed): 1. Route-(a) rejection sound — internal/social/service.go:33-38. Create-path bump must stay unconditional: TestStarUnstarIdempotent pins second-principal → 2 on a nonzero counter, so conditioning the bump on absent/zero undercounts. A late observer whose initial GET sees the winner's just-created record cannot separate it from a stale record with the store CAS alone (record starred_at is RFC3339-seconds wall clock — comparing it to call-start is fragile, and versions are opaque). A CAS-only close would need a protocol change (e.g. record generation epoch) — disproportionate for human-rate Stars. Shard stays; reasoning accepted. 2. Stripe hashing sane — internal/social/social.go:138,146-162. FNV-1a (stdlib) over owner/repo+\x00+principal; fixed [64]sync.Mutex, no map growth/GC; deterministic so same (repo,principal) always shares a shard. Collision = unrelated Stars share a stripe: bounded (~1/64 of Star-only, human-rate traffic), acceptable. 3. Zero-Service path preserved — social.go:179-182 nil-shards no-op; TestStarZeroServiceGateSkips green -race -count=10. 4. Never-nested — single lockStar call site (service.go:50); Unstar takes no gate (version-conditional Delete only), reads lock-free. No path acquires two stripes or re-enters (grep-confirmed). 5. Hold bound honest — lock acquired AFTER requireRead+repoAlive (service.go:40-51, both outside the hold); inside: record GET + record PUT + one ≤8-attempt counter CAS loop (casUpdate attempts=8: social.go:258; reconcileStar:91, bumpStars:161). Reconcile/412 paths hold strictly less. All small control-plane JSON, no LIST/git/bulk. Doc claim '≤1 GET + 1 PUT + ≤8 CAS' verified against all three Star branches incl. resync. 6. Slow-store test genuine AND dispositive — service_test.go:187-238. stallStore wedges Get/Put/Delete but not Head, and repoAlive uses Exists→Head (store.go:231), so the wedge fires at the record GET *inside* the shard, exactly as the test comment claims. Empirical proof: with lockStar emulated as a process-global mutex the new test FAILS ('unrelated Star stalled', 10.0s); with the shipped shards it PASSES -race -count=10. Same-principal/two-repos/different-shards (collision avoided by construction, :190-199); both counters converge to 1. 7. #69 stress green for the right reasons — TestStarConcurrentResyncConverges (8x same jane → one shard → serializes, converges to 1, both absent/zeroed) and TestStarConcurrentConverge (jane+u → two shards → CAS convergence to 2) green -race -count=10. Cross-principal same-repo correctness still rests on CAS, as documented. 8. Hygiene — coverage 99.5% (≥95% gate holds); gofmt clean; go vet clean; no new non-stdlib imports (hash/fnv + sync only, go.mod untouched); full package suite green -race. 9. Docs accurate — 07 §4 rewrite + Decisions entry match the code (shard key, hold bound, regression test names); bound also documented at the lockStar site. 10. 13_concurrency.md untouched — ACCEPTABLE, no amendment required. The gate is feature-level (Service shards), not a RepoHandle lock, so it does not breach the §2 closed repo-lock list; the lock-across-store pattern predates #97 (#69 global gate already in main) and #97 strictly shrinks the hold domain (global → ~1/64 shard) with a documented bound. Non-blocking recommendation: follow-up issue to codify feature-level gate rules in 13 §2 (the 'complete list' sentence is now visibly incomplete) so the next gate does not relitigate this. MERGE RECOMMENDATION: ready to merge. No changes pushed; scratch worktree /tmp/pr106 removed.

crueber closed this issue

2026-09-05 03:31:22 +00:00

crueber referenced this issue from a commit

2026-09-05 03:31:23 +00:00

Merge pull request 'Fix #97: star gate lock-across-store' (#106) from fix/issue-97 into main

crueber commented

2026-09-05 03:31:23 +00:00

Author

Owner

Fixed by PR #106 (review: route-(a) rejection sound, stripes + hold bound verified, slow-store test dispositive; 99.5% coverage), merged. Closing.

crueber added the

fork