klinux / klinux/gocdnext

hardening: completion-path scale + lock-order + terminal-effect ownership

Open
#211 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Go
Stars
7
Forks
3
Avg merge
2h 12m
Merged PRs (30d)
30

Description

Problem

The job-completion + terminalization paths have latent lock-order and scale hazards that a serialized
rollup (needed for correct canceled accounting) can worsen, plus some terminal effects are missing.

Areas

  • Universal intra-table lock order for multi-run ops. Supersede locks victims counter DESC
    (supersede.go), the reaper batch locks by id, cancel batches differ — two txs can grab the same
    runs in opposite orders and deadlock (40P01). Adopt a UNIVERSAL ORDER BY id ASC … FOR UPDATE for
    supersede/reaper/cancel/reconciliation; include the supersede TRIGGER run in the ordered set
    (CompleteJob holds its own run then supersedeAfterCascade locks others). Present victims by counter
    separately from the lock order.
  • Completion-path scale. Serializing every parallel job completion of one run on run FOR UPDATE
    funnels connections onto one row lock (pool exhaustion under wide stages) and lets stale/duplicate/
    old-attempt results queue before losing the CAS. Design: a cheap status/attempt precheck before any
    row lock; the per-completion hot path locks only stage_run (the actual rollup serialization) + the
    job CAS; run FOR UPDATE only on the completion that finalizes the run (once per run) + terminalizers;
    bounded lock_timeout + idempotent retry. Benchmark p95 AND p99 + connection-pool saturation +
    duplicate-result storm
    with a hard budget before rollout.
  • Global lock order incl. deploy tables:
    Lane → Pipeline → Freeze → deploy_target → run → stage → job → artifact → deployment_revision → deploy_watch. StartNativeDeploy/StartNativeDeployDeclared and FinalizeDeployWatch
    (watch→revision today — the odd one out) must be reordered to fit; a fenced DELETE hitting 0 rows ⇒
    no-op. Every path that touches these together adopts it (CompleteJob, approve/reject, cancel, reaper,
    native).
  • Supersede terminalization STAYS synchronous — the at-ready cancel of older lane siblings
    (supersedeLaneSiblings/supersedeOne, supersede.go, already bounded + skip-on-contention) is the
    authorization block for the window where a newer run's gate is READY but not yet PASSED (no
    run_gate_pass marker exists yet, gatepass.go). Do NOT move it to a worker. The only async supersede
    piece is the EXISTING external-effects worker (ClaimSupersedeEffects).
  • Terminal-effect ownership. Manual cancel cleans up but does NOT close the GitHub Check today
    (actions.go — no ReportRunCompleted); the queued/reaper cancel paths that finalize a run discard
    JobCompletion (Check + services stay in_progress). Add per-terminalizer, single-emitter (CAS
    winner), post-commit, service_generation-fenced effects on a detached context
    (context.WithoutCancel + own deadline). The GitHub Check close is best-effort (a stale check is
    cosmetic); the exposed-services: teardown must be DURABLE (outbox/reconciliation) — a leaked exposed
    workload is a security gap, not cosmetic.

Concurrency tests run under a timeout and assert no 40P01, not just correct final state.

Baseline: origin/main. Much of this is pre-existing latent; verify line numbers against current
origin/main.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start from CompleteJob and the named paths in supersede.go, gatepass.go, and actions.go, comparing them with the current origin/main baseline. Run the concurrency tests under a timeout and verify no 40P01 errors, then assess completion, terminalization, lock ordering, and durable service teardown against the stated benchmark and ownership requirements.

Written by the indexing model from the issue text.

Assessment

Tech stack
github, go, postgresql
Domain
backend, ci-cd, databases, distributed-systems, security
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.