Complete back end code review #59
Labels
No labels
actions
bug
cli
duplicate
enhancement
fork
forum
git storage
help wanted
insights
invalid
issues
moderation
oidc
ownership transfer
packages
pr/merge protection rules
projects
pull requests
question
releases
sponsorships
tags
webhooks
wiki
wontfix
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
crueber/walhub#59
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
I want a complete code review of your systems that you have built today. It's. been a long day of coding, and I don't want any back end regressions.
Please post your findings as comments here.
Backend audit (origin/main @
940ca8c) — Q1 deletion + Q2 mutation persistenceMethod: read-only audit from a scratch worktree of
origin/main(/tmp/walhub-main). No checkouts, no commits, no code changes. File:line evidence throughout.Q1 — Repo deletion: bucket sweep is complete, userspace is not
Chain:
DELETE /{o}/{r}/api→internal/api/summary.go:88 repoDelete(admin-gated,AuthAdmin) →cmd/walhub/serve.go:537 repoRegistry.Delete→internal/wal/registry.go:271 Registry.Delete.What the sweep covers (complete):
Registry.Deletetakes the manifest delete as the linearization point first (registry.go:288), then LIST+DELETE-pages everything underrepos/<o>/<r>/(StorePrefix()=git/contract.go:50), thenos.RemoveAllof the local materialization dir (registry.go:314). Since every repo-scoped collab family lives under that prefix, all of these are removed:manifest.pb, WAL segments/packs/log,policy.json,settings,access.json(identity.gokey layout),meta/next_num,meta/labels.json,meta/milestones/*,meta/ci_tokens/*(checks/model.go:134area),meta/import.json(repoimport.go:43-48),meta/social.json,meta/collab_state.json,meta/forks.json,fork.json,meta/invitations/*,issues/*(+index.json),pulls/*(+index.json),checks/*,releases/*(+assets),collab-events/*,webhooks/*(+cursors/deliveries). I verified each family's key constructor emitsrepos/<owner>/<repo>/…(internal/issues,internal/pulls/pulls.go:236-293,internal/notify/notify.go:302-343,internal/social,internal/repoimport).DeleteReleasealso independently cleans its asset prefix (releases/service.go:445-488).Packs: they ARE deleted by the sweep — and that is currently intended, not a leak: fork manifest-sharing is explicitly deferred (
09_rollout.mdWave E notes,pulls/merge.go:runForkForkExecutor nil), so no cross-manifest pack sharing exists for GC to protect yet. When sharing lands, the03 §7GC rule ("referenced by any live manifest") must land with it, and delete must consult it — today there is nothing to consult.Leftovers (by design, no action):
orgs/*(shared across repos — team/org objects must survive one repo's deletion); per-instance leftovers are warmth-only per AGENTS law 4 (other instances' materialized dirs until eviction, 30 s listing cache inregistry.go:338-365, in-memory render LRU, task-table records).Leftover (real gap → filed as issue): repo deletion orphans all userspace references. These live OUTSIDE the swept prefix and nothing cleans them:
users/<p>/starred/<o>/<r>.json+…/watching/…(social/social.go:148-161,notify/notify.go:296-298)users/<p>/notifications/<id>.json+index.jsonreferencing the repo (notify/notify.go:283-296)users/<p>/invitations/index.jsonentries (identity/identity.go:153-155)Consequences, all code-verified:
Starred()lists with no repo-existence check (social/service.go:187-217), so deleted repos linger in starred lists; worse, on delete+recreate the stale star record makesStar()take the early-return path (social/service.go:32-36, no counter bump) while the freshmeta/social.jsoncounter starts at 0 — user shows as starred with desynced counters until an unstar/restar reconverges. Fix directions: lazy-existence filtering on read, or a delete-time tombstone the readers honor.Q2 — Mutation persistence: CAS-then-Create (or CAS) before fan-out everywhere; no early ACK
Spot-checked every mutating service method; the pattern holds uniformly: bucket CAS commits (or returns the store error) BEFORE
updateIndex/emit/stream, and all fan-out helpers are best-effort post-commit (accepted notification-loss per P8), never success-gating:CreateIssue(service.go:139-191: allocNum CAS → thread PutCreate → event Create → index/refs/emits),AddComment(:199-231viaappendEvent),PatchIssue(:297-542, per-field two-steps after full upfront validation),AddReaction/RemoveReaction(:609-727viaappendEvent), labels/milestones viacasUpdate/saveMilestone. Core two-stepstore.go:104-157: header CAS first, event Create second, error (never success) if the Create fails post-CAS.OpenPR(service.go:290-428: counter → thread → event → pr.json Creates, then fan-out, then best-effortrefs/pullpublish with a named repair),UpdatePR(:817-931),appendEvent(:68-112),savePR(:131-162), merge task (merge.go:87-313: ref publish is the commit point, bucketappendEvent+savePR+index converge after, failures narrated loudly instead of rolled back — documented order).SubmitReview(service.go:313-451: header CAS reserves seq+tids → review Create → thread Creates → requester retire → summary → fan-out),DismissReview(same reserve-then-Create),setResolved/AddThreadComment/mutateRequests(header/sidecar CAS loops,threads.go).ReportStatus(service.go:137-266: Create-then-CAS + best-effort index + post-CAS broadcast),CreateToken/RevokeToken(PutCreate/versioned CAS).PutRelease(service.go:59-154: singlecasUpdateincl. tag resolution, then pointer/emit),DeleteRelease,UploadAsset(bytes-first-then-header-CAS, sha/size verified pre-write),DeleteAsset.Star(record Create → counter CAS),Unstar(version-conditional Delete → CAS decrement, exactly-once by version), notifyemit.go(activity-seq reserve → bounded sync fan-out → task fallback), identity access/invite CAS loops.No path returns success before the bucket ACKs. Three robustness observations (not data-loss; no issue filed): (a) multi-field PATCHes (
PatchIssue,UpdatePR,SubmitReview+threads) are not atomic across sub-steps — validation is upfront so only store errors can strand a partial commit, and committed state is always durable; (b) post-commit helper failures invert to error responses (review/threads.go:setResolvedreturnsrefreshSummaryerrors,SubmitReview:440-442returnsremoveRequestererrors) — a client retry of resolve is idempotent, but a retry of submit mints a new review seq (duplicate review on store-error retry); (c)AddReaction's duplicate check (service.go:638-651) is a pre-CAS scan, not revalidated in the mutator, so racing duplicate adds can double-count the summary (human-rate window). One genuine bug found here → filed:pulls/service.go:149-151savePR's CAS-retry does wholesale*cur = *p, so a concurrent body/title update can clobber a just-landed merge outcome (Merged/MergeCommitSHA) back to unmerged — the fix is a field-group merge on retry.Backend audit — Q3 complexity (per package hot-path fan-out) + EVIDENCE E2–E11 coverage
All big-O are per request, counted in bucket round-trips unless noted. Verdict up front: everything is O(1) or O(n) in a bounded small n, except one unbounded read (social
Starred, details below). E2–E11 each measure what they claim; gaps are where a write/fan-out path costs more than its entry's headline numbers.access.jsonGET + 1 GET per referenced team, bounded binding list, exact-key probes, no LISTAddReaction/RemoveReactionrunscanEvents(store.go:171-200: 1 LIST + 1 GET per event) on every write → O(thread length) GETs per reaction. Correct but unbounded in thread length; E3 measures thread reads, not this write path. Accept (human-rate, threads are human-scale) but it is the most expensive reaction write in the treefindOpenPairindex-only;ListPRs1 GET/row page-bounded; GET recomputes head state live (ensurePullHead)UpdatePRmulti-step partial (see Q2 note)pr.json.head_publishedis never persisted (service.go:384-422writes the doc before publish, sets the flag in-memory only) — prod GETs recompute viaensurePullHead, so impact is limited to Git-unwired readers + the POST/GET response skew. Observation onlyrefreshSummary+ gate: O(reviews+threads) GETs + 2 prefix-bounded LISTs, own deadline, no git, never trusts the summarynotifyHeads→openPRHeads(…, 200)(service.go:831-884): index GET + up to 200pr.jsonGETs + up to 200thread.jsonGETs ≈ up to ~400 GETs worst case on a 200-open-PR repo, unmentioned in E6. Bounded (200 cap) and CI-rate, so accept — but the entry should say soemit.go), deduped replays write-flatStarred()(social/service.go:187-217) LISTs the user's whole starred prefix and GETs every record, then truncates to the page — O(total stars) GETs per page load with no backend bound. A 10k-star user costs ~10k GETs per tray page. Keys aren't time-ordered (keyed by repo), so pagination needs either a bound + documented truncation or a time-ordered index. Filed as issueGaps summary: E2–E11 coverage is honest; the three things a reader should not infer from the headlines are (1) reaction writes cost O(thread) GETs, (2) CI reports cost up to ~400 GETs at 200 open PRs, (3) the starred-list read is unbounded. Only (3) is filed (unbounded); (1) and (2) are bounded-and-accepted, recorded here.
Backend audit — Q4 other issues + verdict
Checked and clean (no issue): 401/403 mapping is uniform and correct across all five surfaces —
auth.AuthError→ 401 anonymous / 403 authenticated / 503 unavailable, sentinelsErrUnauthorized→401 / ErrForbidden→403 / ErrNotFound→404 / ErrInvalid→400 / ErrConflict→409 / ErrUnprocessable→422, and every package'swritePlainsetsWWW-Authenticate: Beareron 401 andRetry-After: 15on 503 (issues/http.go,pulls/http.go,review/http.go,checks/http.go— identical envelopes). The statuses that erase git credentials (real 401s on bad CI token / revoked token,checks/service.go:174-182) behave. Validation is upfront everywhere I looked (titles/bodies/labels/colors/milestones/reactions/anchors/SHAs/tags/cursors all validated pre-mutation; strict unknown-key rejection at the HTTP layer). Pagination is clamped (n>100clamps,TrayMaxPage,ListMaxPage/ListScanCap, event windows capped 200). Token minting is admin-only with PutCreate + collision retry.Unstaris exactly-once via version-conditional delete (social/service.go:56-76). No lock-held-across-I/O smells in the feature packages (CAS loops only, bounded attempts everywhere).issues statusFordefaults unknown errors to 503 while pulls/review default to 500 — deliberate-looking (store-first bias), not filed.Observations (verified, accepted, not filed): reaction duplicate-check race; submit-partial → duplicate-on-retry; post-commit helper errors inverting committed work into 5xx (resolve is idempotent-safe);
head_publishednever persisted (self-heals viaensurePullHeadon every GET); render-cache aliasing across delete+recreate at identical manifest revision (in-memory LRUapi/cache.go:189-219+ bucket mirrorcache/api/v1/*which also survives deletion outside the repo prefix — stale until the next revision bump);savePR-style wholesale overwrite exists only in pulls (filed).Filed issues (3, conservative — each code-verified with impact + fix direction):
savePRCAS-retry wholesale overwrite can clobber a landed merge outcome.Starredlist is an unbounded GET-per-record scan.Verdict for #59
Starredread (filed); E2–E11 headlines hold with the three noted undercounts.No regressions found that endanger the backend. Nothing was changed in the tree (read-only audit; main worktree untouched).
Audit checklist complete: Q1 deletion verified complete except userspace (fixed #63, merged); Q2 persistence holds everywhere, one real race found+fixed (#64, merged); Q3 O(1)/bounded except Starred scan (fixed #65, merged) + Star race flake found during review (fixed #69, merged); Q4 findings posted above, no other changes requested. All follow-ups merged. Closing.