Skip to content

Implement RedisLock: replace stub with full go-redis/v9 distributed lock#48

Merged
intel352 merged 4 commits intomainfrom
copilot/implement-redis-lock
Feb 22, 2026
Merged

Implement RedisLock: replace stub with full go-redis/v9 distributed lock#48
intel352 merged 4 commits intomainfrom
copilot/implement-redis-lock

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Feb 22, 2026

RedisLock in scale/distributed_lock.go was a non-functional stub returning errors on every call. This implements it fully using github.com/redis/go-redis/v9.

Core implementation

  • AcquireSET key <token> NX PX <ttl> with exponential backoff (16ms→512ms cap) until acquired or ctx cancelled
  • TryAcquire — single SET NX attempt; returns (nil, false, nil) cleanly when held (no error)
  • Release — Lua check-and-delete ensures only the token holder can release; sync.Once-guarded for idempotency
  • Close() — closes the underlying Redis client connection
  • Lazy client init via sync.Once on first Acquire/TryAcquire call

New constructor

// Original still works (password="", db=0)
lock := NewRedisLock("localhost:6379")

// Full options
lock := NewRedisLockWithOptions("localhost:6379", "password", 1)

Dependencies

  • github.com/redis/go-redis/v9 promoted from indirect → direct
  • github.com/alicebob/miniredis/v2 added for tests (in-memory Redis, no real server needed)

Tests

Replaces the stub-error test with coverage of: acquire+release cycle, TryAcquire free/held, idempotent release, wrong-holder Lua safety, TTL expiry via miniredis.FastForward, and context cancellation.

Original prompt

Problem

The scale/distributed_lock.go file contains a RedisLock struct that is a complete stub. Both Acquire() and TryAcquire() return "redis lock not implemented" errors. The PostgreSQL advisory lock implementation (PGAdvisoryLock) in the same file is fully functional, but Redis is not.

Current Code

// scale/distributed_lock.go

// RedisLock implements DistributedLock using Redis SET NX with TTL.
// Requires a Redis client connection. This is a stub implementation;
// the full implementation will be provided when the Redis client is
// integrated as a direct dependency.
type RedisLock struct {
    addr string
}

func NewRedisLock(addr string) *RedisLock {
    return &RedisLock{addr: addr}
}

func (l *RedisLock) Acquire(_ context.Context, key string, _ time.Duration) (func(), error) {
    return nil, fmt.Errorf("redis lock not implemented: configure Redis client for key %s at %s", key, l.addr)
}

func (l *RedisLock) TryAcquire(_ context.Context, key string, _ time.Duration) (func(), bool, error) {
    return nil, false, fmt.Errorf("redis lock not implemented: configure Redis client for key %s at %s", key, l.addr)
}

Requirements

  1. Add github.com/redis/go-redis/v9 to go.mod.

  2. Implement RedisLock with proper distributed locking:

    • Acquire(ctx, key, ttl):
      • Use SET key value NX PX ttl_ms to acquire the lock.
      • The value should be a unique token (UUID or random string) to ensure only the holder can release.
      • If the lock is already held, retry with exponential backoff until context is cancelled.
      • Return a release function that uses a Lua script to atomically check-and-delete:
        if redis.call("get", KEYS[1]) == ARGV[1] then
            return redis.call("del", KEYS[1])
        else
            return 0
        end
    • TryAcquire(ctx, key, ttl):
      • Same as Acquire but only try once — no retry.
      • Return (release, true, nil) if acquired, (nil, false, nil) if not acquired (no error).
  3. Add a Close() method to clean up the Redis client connection.

  4. Add connection configuration:

    • Support addr (already present), password, db (database number).
    • Create the Redis client in a Connect() or lazily on first Acquire.
  5. Write tests in scale/distributed_lock_test.go:

    • Use github.com/alicebob/miniredis/v2 for an in-memory Redis server (no real Redis needed).
    • Test Acquire + release cycle.
    • Test TryAcquire when lock is free → acquires.
    • Test TryAcquire when lock is held → returns false without error.
    • Test that release function is idempotent (calling twice doesn't panic or error).
    • Test that a different holder cannot release the lock (Lua script safety).
    • Test TTL expiry (miniredis supports FastForward).
  6. Ensure all existing tests pass (go test ./...).

  7. Follow the same DistributedLock interface that PGAdvisoryLock implements in the same file.

This pull request was created from Copilot chat.


🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

…s tests

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Copilot AI changed the title [WIP] Implement RedisLock for distributed locking functionality Implement RedisLock: replace stub with full go-redis/v9 distributed lock Feb 22, 2026
Copilot AI requested a review from intel352 February 22, 2026 19:42
@intel352 intel352 marked this pull request as ready for review February 22, 2026 20:16
@intel352 intel352 merged commit 01e5e22 into main Feb 22, 2026
14 checks passed
@intel352 intel352 deleted the copilot/implement-redis-lock branch February 22, 2026 20:27
intel352 added a commit that referenced this pull request Apr 24, 2026
…egistry, typed gRPC args, migrate image, teardown

Five features bundled into v0.19.0 for shared config-file shape (wfctl.yaml +
.wfctl-lock.yaml) and release boundary. Each addresses architectural debt
surfaced during BMW tonight's deploy blocker chain.

Features:
- A. Plugin manifest + lockfile split (tasks #42/#43)
- B. Multi-registry + IaCProvider.EnsureRegistryAuth (task #48)
- C. Typed-args refactor for IaCProvider gRPC (task #41)
- D. Official workflow-migrate Docker image (task #49)
- E. wfctl infra teardown with mandatory dry-run + --approve flag (new)

Non-goals: constraint-based plugin resolution (v0.20.0), transitive plugin
deps, OCI chart/artifact registries, cross-registry mirroring.

Autonomous pipeline target: v0.19.0 after BMW post-teardown stabilizes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
intel352 added a commit that referenced this pull request Apr 24, 2026
* docs: v0.19.0 architectural cleanup design — plugin manifest, multi-registry, typed gRPC args, migrate image, teardown

Five features bundled into v0.19.0 for shared config-file shape (wfctl.yaml +
.wfctl-lock.yaml) and release boundary. Each addresses architectural debt
surfaced during BMW tonight's deploy blocker chain.

Features:
- A. Plugin manifest + lockfile split (tasks #42/#43)
- B. Multi-registry + IaCProvider.EnsureRegistryAuth (task #48)
- C. Typed-args refactor for IaCProvider gRPC (task #41)
- D. Official workflow-migrate Docker image (task #49)
- E. wfctl infra teardown with mandatory dry-run + --approve flag (new)

Non-goals: constraint-based plugin resolution (v0.20.0), transitive plugin
deps, OCI chart/artifact registries, cross-registry mirroring.

Autonomous pipeline target: v0.19.0 after BMW post-teardown stabilizes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs: v0.19.0 design — add Features F, G, H (outputs, verify, secret sinks)

Scope expanded from 5 to 7 features per user feedback on BMW CI gap audit:
- F. wfctl infra outputs with masked-by-default sensitivity + GHA ::add-mask::
- G. wfctl deploy verify with multi-target healthcheck + retry/timeout gate
- H. Declarative secret sinks (outputs.<field>.sinks[]) — plaintext never
  leaves wfctl process; built-in github_secret + github_env handlers;
  aws/gcp/azure sinks via plugin fan-out in v0.19.x

Motivation: BMW's Capture staging DB URL step uses doctl + awk + gh secret
set shell pipeline, leaking DATABASE_URL plaintext through stdout/env/argv.
Declarative sink pattern (like terraform's output-to-secret-manager) writes
the value in-process directly to the GitHub secrets API with libsodium
encryption. Matches user's stated principle: "if BMW CI has provider-specific
shell, fix it in workflow/wfctl so the CI stays declarative."

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs: v0.19.0 implementation plan — 7 features × 9 phases

Matches design doc 2026-04-24-v0.19.0-architectural-cleanup-design.md:
- Phase 1 alpha.1: Feature A (plugin manifest + lockfile)
- Phase 2 alpha.2: Feature C client-side (typed gRPC args)
- Phase 3 (DO plugin v0.8.0): Feature C server-side + integration tests
- Phase 4 alpha.3: Feature B (multi-registry)
- Phase 5 (DO plugin v0.8.1): Feature B server-side (EnsureRegistryAuth)
- Phase 6a rc1: Feature D (workflow-migrate image)
- Phase 6b rc2: Feature E (wfctl infra teardown)
- Phase 6c rc3: Features F + G + H (outputs + verify + sinks)
- Phase 7: v0.19.0 final + changelog + docs
- Phase 8: Plugin fan-out (aws/gcp/azure/tofu) in parallel
- Phase 9: BMW migration PR (after v0.19.0 stabilizes)

Timing: all phases can merge independently; final v0.19.0 tag and Phase 9
hold until BMW's tonight deploy chain reaches prod /healthz green (task #26).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs: address PR #474 review — reconcile feature count, flag naming, source task column

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
intel352 added a commit that referenced this pull request Apr 24, 2026
…nfra_output (#476)

* docs: v0.19.0 architectural cleanup design — plugin manifest, multi-registry, typed gRPC args, migrate image, teardown

Five features bundled into v0.19.0 for shared config-file shape (wfctl.yaml +
.wfctl-lock.yaml) and release boundary. Each addresses architectural debt
surfaced during BMW tonight's deploy blocker chain.

Features:
- A. Plugin manifest + lockfile split (tasks #42/#43)
- B. Multi-registry + IaCProvider.EnsureRegistryAuth (task #48)
- C. Typed-args refactor for IaCProvider gRPC (task #41)
- D. Official workflow-migrate Docker image (task #49)
- E. wfctl infra teardown with mandatory dry-run + --approve flag (new)

Non-goals: constraint-based plugin resolution (v0.20.0), transitive plugin
deps, OCI chart/artifact registries, cross-registry mirroring.

Autonomous pipeline target: v0.19.0 after BMW post-teardown stabilizes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs: v0.19.0 design — add Features F, G, H (outputs, verify, secret sinks)

Scope expanded from 5 to 7 features per user feedback on BMW CI gap audit:
- F. wfctl infra outputs with masked-by-default sensitivity + GHA ::add-mask::
- G. wfctl deploy verify with multi-target healthcheck + retry/timeout gate
- H. Declarative secret sinks (outputs.<field>.sinks[]) — plaintext never
  leaves wfctl process; built-in github_secret + github_env handlers;
  aws/gcp/azure sinks via plugin fan-out in v0.19.x

Motivation: BMW's Capture staging DB URL step uses doctl + awk + gh secret
set shell pipeline, leaking DATABASE_URL plaintext through stdout/env/argv.
Declarative sink pattern (like terraform's output-to-secret-manager) writes
the value in-process directly to the GitHub secrets API with libsodium
encryption. Matches user's stated principle: "if BMW CI has provider-specific
shell, fix it in workflow/wfctl so the CI stays declarative."

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs: v0.19.0 implementation plan — 7 features × 9 phases

Matches design doc 2026-04-24-v0.19.0-architectural-cleanup-design.md:
- Phase 1 alpha.1: Feature A (plugin manifest + lockfile)
- Phase 2 alpha.2: Feature C client-side (typed gRPC args)
- Phase 3 (DO plugin v0.8.0): Feature C server-side + integration tests
- Phase 4 alpha.3: Feature B (multi-registry)
- Phase 5 (DO plugin v0.8.1): Feature B server-side (EnsureRegistryAuth)
- Phase 6a rc1: Feature D (workflow-migrate image)
- Phase 6b rc2: Feature E (wfctl infra teardown)
- Phase 6c rc3: Features F + G + H (outputs + verify + sinks)
- Phase 7: v0.19.0 final + changelog + docs
- Phase 8: Plugin fan-out (aws/gcp/azure/tofu) in parallel
- Phase 9: BMW migration PR (after v0.19.0 stabilizes)

Timing: all phases can merge independently; final v0.19.0 tag and Phase 9
hold until BMW's tonight deploy chain reaches prod /healthz green (task #26).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs: address PR #474 review — reconcile feature count, flag naming, source task column

* docs: v0.18.9 phase-continuation design — env-resolution consistency

BMW deploy run 24888583717 created a duplicate DO App Platform app because
wfctl infra apply used env-resolved name "bmw-staging" while wfctl ci run
--phase deploy used base module name "bmw-app". Both paths call driver.Read
by name; with different names they find different resources (or none) and
each calls Create, producing duplicates.

Root cause: cmd/wfctl/deploy_providers.go:769 reads m.Name directly after
ResolveForEnv has been applied. Same class as v0.18.7's Task #32 fix but
in the deploy-phase code path.

Fix: refactor resolveModCfg closure to return *ResolvedModule, use
resolved.Name at call sites. Audit + patch infra_output source resolution
(task #56) with the same pattern. Ship as v0.18.9.

Does not require state-sharing between IaC and CI phases; the bug is about
names, not state. Both phases use driver.Read by name; aligning the names
aligns the lookups.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs: v0.18.9 phase-continuation implementation plan

9 tasks across Phase 1 (core fixes: deploy_providers.go + infra_secrets.go
+ regression tests) and Phase 2 (release + BMW unblock: PR, merge, tag,
BMW bump, teardown, redeploy).

Same-class fix as v0.18.7 Task #32: env-resolved Name used consistently
wherever modules are consumed. Target: v0.18.9 hotfix; unblocks BMW
staging deploy from run 24888583717 duplicate-resource failure.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(wfctl): ci run deploy uses env-resolved module name (not base)

Refactored resolveModCfg closure in deploy_providers.go to return
*config.ResolvedModule so callers see both resolved.Name (env-override
lifted from Config["name"]) and resolved.Config. All three call sites
(iac.provider lookup, findByType, fallback loop) now read resolved.Name
instead of m.Name.

Same class as v0.18.7 Task #32 fix for ResourceSpec.Name — env override
of Config["name"] was lifted into ResolvedModule.Name but deploy_providers.go
read m.Name directly, ignoring the override. Caused BMW deploy run
24888583717 to create duplicate DO apps (bmw-app vs bmw-staging).

Regression tested via:
- TestPluginDeployProvider_UsesEnvResolvedName (new, was failing)
- TestPluginDeployProvider_FallsBackToModuleNameWhenNoEnv (new, baseline)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(wfctl): infra_output source module name flows through env resolution

Introduces resolveInfraOutput(wfCfg, source, envName, stateOutputs)
which translates the base module name in a "module.field" source string
to its env-resolved name before looking up state. State is persisted
under the env-resolved name (e.g. "bmw-staging-db"), so "bmw-database.uri"
with --env staging now correctly finds the state entry.

syncInfraOutputSecrets now accepts wfCfg and envName so the new
resolution is applied for every infra_output secret in the generate list.
The call site in infra.go (runInfraApply) loads the workflow config and
passes it through.

Closes task #56. Regression tested via:
- TestInfraOutput_EnvResolvesModuleSource (new, was failing)
- TestInfraOutput_NoEnvUsesBaseName (new, baseline)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* docs: CHANGELOG v0.18.9 entry

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(wfctl): stateKeys actually sorts keys (comment matched implementation)

Agent-Logs-Url: https://github.com/GoCodeAlone/workflow/sessions/a0429849-a053-4485-914d-ccb115be94e8

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

* fix(wfctl): address 4 Copilot round-1 findings on v0.18.9 (#476)

- resolveInfraOutput: ResolveForEnv ok=false now errors (config error)
  instead of silently falling back to base module name — prevents
  the env-resolution fix from being bypassed on misconfigured envs
- stateKeys: add sort.Strings so error messages list available modules
  in deterministic order (comment already said "sorted")
- infra.go: surface config.LoadFromFile error instead of discarding it —
  silent failure would regress env resolution to the pre-fix nil-wfCfg path
- CHANGELOG: replace "Closes task #60" (ambiguous GitHub issue ref) with
  "Root cause from BMW deploy run 24888583717"

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(wfctl): accurate error message + test for explicitly-disabled module in resolveInfraOutput

Agent-Logs-Url: https://github.com/GoCodeAlone/workflow/sessions/3accbfdf-259b-4b98-a44e-8b538d3f5857

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

* fix(wfctl): gate LoadFromFile on envName + infra_output presence (#476)

Skip config.LoadFromFile when env resolution is not needed:
- envName="" → no env resolution, wfCfg=nil is correct
- no infra_output generators → syncInfraOutputSecrets ignores wfCfg

Avoids unnecessary file I/O on every infra apply when the caller
has no infra_output secrets or is not running with --env.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants