[cristim]

Summary

ReleaseAdvisoryLock ran its unlock query on the caller's own context. When that context is already canceled or past its deadline by the time the deferred release runs (a scheduled task whose Lambda invocation context, or HTTP request context, expired while dispatchTask was still in flight), the unlock query fails immediately, and the otherwise-healthy pinned connection still goes back to the pool with the session-level advisory lock still held. Every subsequent invocation of that task type then gets acquired=false and returns {"status":"skipped","reason":"already_running"} with HTTP 200 until that specific pooled connection happens to be recycled -- collection (or whichever task) stops silently and stays stopped.

Root cause

internal/database/connection.go's ReleaseAdvisoryLock issued SELECT pg_advisory_unlock($1) on the caller's ctx, then unconditionally deferred conn.Release() regardless of whether the unlock actually ran. The scheduler's own deferred marker-clear (internal/scheduler/scheduler.go, CollectRecommendations) already detaches onto context.Background() with a short timeout for exactly this reason; this call site diverged from that established pattern.

Fix

Both changes from the issue's fix direction:

1. Detach the unlock query onto a fresh context.Background() with a 5s timeout (advisoryLockReleaseTimeout), matching the scheduler's pattern, so a dead caller context can no longer skip the unlock.
2. When the unlock query itself errors (not just "lock not held", which is a successful confirmation via the released bool), close the underlying *pgx.Conn before the deferred Release() runs. pgxpool.Conn.Release() already destroys (rather than pools) a connection it finds closed, so a connection that might still hold the lock can never be returned to the pool regardless of what specifically caused the unlock query to fail.

Regression test

internal/database/advisory_lock_integration_test.go (build tag integration, package database_test):

• Acquires the lock via TryAdvisoryLock.
• Calls ReleaseAdvisoryLock with an already-canceled context, simulating the dead-invocation-context scenario.
• Verifies from a second, independent raw connection (not container.DB's own pool) that the lock is free. This matters: pg_try_advisory_lock is reentrant per session, so re-asking the same connection pool could spuriously pass by reacquiring the exact pooled backend session that never actually released the lock.

This test genuinely could not be run locally: Docker is unavailable in this environment (per the project's local-verification constraints for this session). It is gated behind the same testcontainers-backed Integration Tests CI job every other DB-container test in this repo already relies on, and compiles cleanly with the integration build tag (go build -tags integration ./..., go vet -tags integration ./...).

Verification

• GOWORK=off go build ./... — pass
• GOWORK=off go build -tags integration ./... — pass
• GOWORK=off go vet ./... and -tags integration — pass
• GOWORK=off go test ./... -short — 6133 tests pass
• GOWORK=off go test ./internal/database/... -race -short -run TestReleaseAdvisoryLock — pass
• GOWORK=off go mod tidy -diff — empty
• gofmt -l on changed files — clean
• golangci-lint at the CI-pinned v2.10.1: reproduces the same pre-existing local toolchain issue noted on PR fix(frontend): RI-exchange execute never sends region, so the backend returns 400 #355/fix(api): PUT /api/plans/{id} resets the ramp schedule and re-buys completed steps #360/fix(api): "Run now" flips a purchase to running but nothing executes it #370 for this checkout, unrelated to this diff; relying on CI's clean go1.26.6 environment for the authoritative lint run.

Note

The issue also suggests surfacing a reason=already_running response that persists past a plausible run duration, rather than always returning it as a 200. That's a separate observability improvement (detecting staleness, not fixing the underlying lock leak), so it's left out of this focused fix; flagging it here in case it's worth a follow-up issue.

Closes #105

Summary by CodeRabbit

• Bug Fixes
  • Advisory locks are now released even if the original request is canceled, helping prevent locks from lingering and blocking later operations.
  • If releasing a lock fails, the affected connection is removed from circulation to avoid returning a potentially locked connection to the pool.
  • Lock release now has a time limit, so cleanup attempts do not wait indefinitely.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml

Review profile: CHILL

Plan: Essentials

Run ID: 53efa1ad-036a-4b89-af6e-96bb8cbd4872

📥 Commits

Reviewing files that changed from the base of the PR and between e5932ba and 174b951.

📒 Files selected for processing (1)

• internal/database/connection.go

🚧 Files skipped from review as they are similar to previous changes (1)

• internal/database/connection.go

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour.

---

📝 Walkthrough

Walkthrough

ReleaseAdvisoryLock now uses a background context with a five-second timeout for the unlock query. It closes the connection if the query fails. An integration test checks that another session can acquire the lock after release when the caller context is canceled.

Changes

Advisory lock release

Layer / File(s)	Summary
Detached unlock and cancellation test internal/database/connection.go, internal/database/advisory_lock_integration_test.go	The unlock query uses a background context limited to five seconds. The connection closes if the query returns an error. The integration test checks lock acquisition from a separate session after release with a canceled caller context.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 174b9

The lock cleanup change is mergeable after normal checks; the integration test has not been run in the supplied evidence.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the advisory-lock cleanup issue and its resulting permanent already_running state. It accurately reflects the main change.
Linked Issues check	✅ Passed	Issue #105 requires detached advisory-lock cleanup and connection destruction when the unlock query fails. ReleaseAdvisoryLock now uses a fresh background context with a five-second timeout. It atte…
Out of Scope Changes check	✅ Passed	The changes stay within issue #105. The production change updates advisory-lock cleanup, and the integration test covers the canceled-context regression. No unrelated change is identified in the suppl…
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files.

✨ Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Independent adversarial review + local verification: MERGE at 174b951. The advisory-lock unlock now runs on a detached 5s context (a query on an already-canceled ctx never reaches Postgres, pgconn.go:553-556), and on unlock failure the pinned conn is closed so pgxpool destroys rather than re-pools it (pgxpool v5.9.2 conn.go:33). The only session-level lock caller; xact locks elsewhere are unaffected. Pre-fix proof run against a real local Postgres 17 (lock still held after release on a canceled ctx); the CI integration test executed and passed. Follow-up: surface a persistent already_running as non-200.