fix(worker): reclaim expired 'processing' leases from departed workers

[sanastasiou]

Problem

_reclaim_own_processing_tasks matches worker_id = <own id>, so startup recovery can only rescue rows this worker abandoned. But worker_id defaults to the container hostname:

# hindsight_api/worker/main.py:225
worker_id = args.worker_id or socket.gethostname()

In Docker every compose down/up mints a new container id, so anything in flight at that moment is stranded in processing under an id that will never return — and no future startup can match it. claimed_at is already recorded on claim but is never read, so nothing else notices either.

Consolidation turns that from lossy into permanent. It is serialised per bank at claim time (busy_bank_ids in claim_tasks), so a single corpse in processing marks its bank busy forever and no consolidation for that bank can ever be claimed again. With the default {"consolidation": 2} reservation, those slots then sit idle indefinitely.

Evidence from production

Single-container deployment, v0.9.1. Two consolidations stranded on 2026-07-05 were still processing 50 days later:

Worker: 5def1aaf9f8a (2 task(s))
  46e1a986  consolidation  bank=skynet-brain     running=50 days, 7:37:18
  260cf73c  consolidation  bank=stefanos-family  running=50 days, 7:32:12

The worker had been at half capacity that entire time — two slots reserved for work that could never be claimed, while retain queued behind the two shared slots:

slots=2/4 | reserved: [consolidation=0/2(avail=2)] | shared=2/2(avail=0)

hindsight-admin worker-status showed six departed container ids holding nine rows in total — one per historical redeploy. hindsight-admin decommission-worker <id> cleared them and slots went to 4/4 immediately:

slots=4/4 | reserved: [consolidation=2/2(avail=0)] | shared=2/2(avail=0)

So the recovery primitive already exists and works — it just isn't reachable automatically, and an operator has no reason to look until something is visibly broken. Nothing alerts: the API stays healthy, the queue keeps moving on the shared slots, and the only symptom is throughput quietly halving.

Worth noting the API can't help either — both affordances refuse processing rows:

409  cannot be retried: status is 'processing', expected 'failed' or 'cancelled'
409  cannot be deleted: status is 'processing', expected 'failed', 'cancelled' or 'completed'

Change

A startup sweep alongside the existing own-rows recovery: reclaim processing rows whose claimed_at is older than HINDSIGHT_API_WORKER_ORPHAN_LEASE_SECONDS (default 86400), regardless of owner.

Guards:

• This worker's own rows are excluded — the caller directly above already handles them, with retry/fail accounting.
• Batch API rows are excluded, for the same reason as the own-rows path: long-lived by design.
• Only expired leases — 24 h default is far beyond any real task, and the rows above were 50 days old. A live worker's fresh row is never touched.
• 0 disables the sweep entirely.

The log line names the departed owners and points at HINDSIGHT_API_WORKER_ID, so operators fix the root cause rather than lean on the sweep:

Worker live-worker reclaimed 9 operation(s) abandoned by 6 departed worker(s) ...
Set HINDSIGHT_API_WORKER_ID to a stable value so worker ids survive container
recreation and this is not needed.

Relationship to existing issues

• Operations orphaned by a worker restart are unrecoverable through the API #3448 fixed this class for the case where the worker id is stable. This covers the case where it is not — which, with the hostname default, is every Docker restart.
• Async operations orphaned permanently after container recreation (worker id defaults to hostname) #3709 describes this exact gap (closed not_planned).
• Operations stuck in 'processing' after max retries exhausted (zombie operations) #3594 (zombie operations after retries exhausted) is the same shape from a different entry point.
• fix(worker): detect stuck tasks by stage progress #3720 (detect stuck tasks by stage progress) is a finer-grained detector; a lease bound is the coarse backstop underneath it.

I'd also gently suggest warn_if_container_default_worker_id deserves to be louder. It fired on every one of our boots for 50 days and we sailed past it — a warning that predicts a 50 % capacity loss is arguably an error, or at least worth repeating periodically rather than once at startup.

Tests

Three cases added to TestWorkerRecovery, following the existing test_recover_own_tasks_does_not_affect_other_workers pattern:

• test_expired_lease_from_departed_worker_is_reclaimed
• test_fresh_lease_from_another_worker_is_left_alone
• test_orphan_sweep_is_disabled_when_lease_is_zero

tests/test_worker.py: 110 passed, ruff check clean. (2 unrelated setup errors locally — missing optional sentence_transformers in a minimal env.)

---

Update — heartbeat-aware liveness (7d96b38)

The Strix review caught a real race in the first version, and I've taken the fix.

The sweep originally judged abandonment on claimed_at alone. That column is written once at claim time and never refreshed — but a long-running job does emit progress heartbeats, and those land on updated_at (MemoryEngine._write_operation_progress, memory_engine.py:3579). Combined with consolidation having no wall-clock ceiling (_wall_timeout_for returns None for everything except retain), a healthy consolidation can legitimately outlive a 24 h lease — and resetting it to pending mid-flight would let a second worker claim the same bank and run it concurrently.

The sweep now requires both timestamps to be stale. A job that is merely slow is never touched; only one that is old and silent.

Worth noting: this change made my own test_expired_lease_from_departed_worker_is_reclaimed fail, and that was the test being unfaithful rather than the fix being wrong. It set a stale claimed_at but let updated_at default to now(). Real abandoned rows have both stale — the two production corpses had created == updated == 2026-07-05T14:31:37, never touched again, because no heartbeat ever ran. The fixture now matches.

New test: test_slow_but_live_job_with_fresh_heartbeat_is_not_reclaimed — 50-day-old claim, heartbeat seconds ago, different worker → must not be reclaimed.

tests/test_worker.py: 111 passed, ruff clean.

Two adjacent points from the same review — unfenced terminal writes (_mark_completed matches operation_id + status with no worker_id guard) and unbounded consolidation — I agree with, but both are pre-existing and independent of this change, so they're left out to keep the diff reviewable. Happy to fold either in if maintainers prefer.

[strix-security]

Strix Security Review

Warning

This pull request has 2 commits after the last Strix review (222a4b8). Strix has not reviewed these changes.
Automatic review on push is off for this repository. To review the latest changes, tag @strix-security in a comment, or turn on re-review on push.

1 open security finding on this PR:

• 🔵 MEDIUM Race condition in orphan-lease reclaim resets live long-running consolidations to pending, enabling concurrent duplicate execution — hindsight-api-slim/hindsight_api/worker/poller.py:1207-1212

Review summary

Reviewed the four changed files in this PR (config, worker entrypoint, worker poller, and tests). The new startup orphan sweep is parameterized and correctly scopes out the caller's own rows and batch-API rows, and the configuration plumbing is benign. The substantive concern is in _reclaim_expired_processing_tasks: it treats the claim-time claimed_at column as a lease, even though that column is never refreshed by a heartbeat. The codebase already emits progress heartbeats into updated_at for long-running consolidations, but the sweep ignores that signal, so a healthy consolidation that legitimately outlives the default 24-hour lease can be reset to pending and re-claimed while its original worker is still executing, defeating the existing per-bank/document serialization and enabling concurrent duplicate execution. The confirmed finding is "Race condition in orphan-lease reclaim resets live long-running consolidations to pending, enabling concurrent duplicate execution" (see the filed report for the full technical analysis and remediation).

_{Fixed the findings? re-run the review, or tag @strix-security in a PR comment to run a fresh review.}

_{Updated for 222a4b8.}

---

Reviewed by Strix
_{Re-run review · Configure security review settings}

[strix-security]

Strix flagged a new security finding below. See the pinned summary comment for the full PR status.

[strix-security]

🔵 Race condition in orphan-lease reclaim resets live long-running consolidations to pending, enabling concurrent duplicate execution

Severity: MEDIUM · CWE-362

The startup reclaim sweep added by this PR (_reclaim_expired_processing_tasks) decides whether a processing operation has been abandoned using only claimed_at, a timestamp written once at claim time and never refreshed. Long-running jobs do not update claimed_at; they emit progress heartbeats that bump updated_at instead (MemoryEngine._write_operation_progress, called by the consolidator at phase/batch boundaries).

Consolidation has no wall-clock timeout (_wall_timeout_for returns None for consolidation) and HINDSIGHT_API_CONSOLIDATION_MAX_MEMORIES_PER_ROUND may be set to 0 (unlimited), so a single consolidation operation can legitimately run past the default 24-hour lease. When another worker starts while that job is still running — a rolling deploy, scale-out, or recovery of a peer — the sweep matches the row (status = 'processing', a different worker_id, stale claimed_at) and resets it to pending, clearing worker_id and claimed_at.

Nothing re-checks ownership mid-flight, and the claim predicates (bank_serialization_sql / document_serialization_sql) enforce exclusivity keyed on status = 'processing'. Once the row is back to pending, a second worker can claim the same bank/document and run the operation concurrently with the first worker, causing duplicate LLM inference and competing writes to the same bank or document (lost updates / duplicated memories). Terminal writes are also unfenced: _mark_completed matches only operation_id + status = 'processing' (no worker_id guard), so the original worker can mark the row completed even after a second worker has re-claimed and is still running it.

Suggested change

	WHERE status = 'processing'
	AND worker_id IS NOT NULL
	AND worker_id <> $1
	AND claimed_at IS NOT NULL
	AND claimed_at < now() - make_interval(secs => $2)
	AND result_metadata->>'batch_id' IS NULL
	WHERE status = 'processing'
	AND worker_id IS NOT NULL
	AND worker_id <> $1
	AND claimed_at IS NOT NULL
	AND claimed_at < now() - make_interval(secs => $2)
	AND updated_at < now() - make_interval(secs => $2)
	AND result_metadata->>'batch_id' IS NULL

Prompt to fix with AI

This is a security vulnerability found during a code review.

Vulnerability: Race condition in orphan-lease reclaim resets live long-running consolidations to pending, enabling concurrent duplicate execution
Severity: MEDIUM
CWE: CWE-362

The startup reclaim sweep added by this PR (`_reclaim_expired_processing_tasks`) decides whether a `processing` operation has been abandoned using only `claimed_at`, a timestamp written once at claim time and never refreshed. Long-running jobs do not update `claimed_at`; they emit progress heartbeats that bump `updated_at` instead (`MemoryEngine._write_operation_progress`, called by the consolidator at phase/batch boundaries).

Consolidation has no wall-clock timeout (`_wall_timeout_for` returns `None` for `consolidation`) and `HINDSIGHT_API_CONSOLIDATION_MAX_MEMORIES_PER_ROUND` may be set to `0` (unlimited), so a single consolidation operation can legitimately run past the default 24-hour lease. When another worker starts while that job is still running — a rolling deploy, scale-out, or recovery of a peer — the sweep matches the row (`status = 'processing'`, a different `worker_id`, stale `claimed_at`) and resets it to `pending`, clearing `worker_id` and `claimed_at`.

Nothing re-checks ownership mid-flight, and the claim predicates (`bank_serialization_sql` / `document_serialization_sql`) enforce exclusivity keyed on `status = 'processing'`. Once the row is back to `pending`, a second worker can claim the same bank/document and run the operation concurrently with the first worker, causing duplicate LLM inference and competing writes to the same bank or document (lost updates / duplicated memories). Terminal writes are also unfenced: `_mark_completed` matches only `operation_id` + `status = 'processing'` (no `worker_id` guard), so the original worker can mark the row `completed` even after a second worker has re-claimed and is still running it.

Location: hindsight-api-slim/hindsight_api/worker/poller.py:1207-1212
Context: Require the progress heartbeat (updated_at) to also be stale before reclaiming
```
// Before:
                WHERE status = 'processing'
                  AND worker_id IS NOT NULL
                  AND worker_id <> $1
                  AND claimed_at IS NOT NULL
                  AND claimed_at < now() - make_interval(secs => $2)
                  AND result_metadata->>'batch_id' IS NULL
// After:
                WHERE status = 'processing'
                  AND worker_id IS NOT NULL
                  AND worker_id <> $1
                  AND claimed_at IS NOT NULL
                  AND claimed_at < now() - make_interval(secs => $2)
                  AND updated_at < now() - make_interval(secs => $2)
                  AND result_metadata->>'batch_id' IS NULL
```

How to fix:
Base liveness on the existing progress heartbeat rather than the static claim time: require the row's `updated_at` to also be older than the lease before reclaiming (the progress heartbeat bumps `updated_at` at phase/batch boundaries), or refresh `claimed_at` in `_write_operation_progress` on each heartbeat. Add `worker_id` (or an explicit lease token) fencing to the terminal-write statements in `_mark_completed` / `_mark_failed` so a worker cannot complete or fail an operation it no longer owns. Additionally, consider bounding consolidation with a wall-clock timeout (as `retain` already is) so a single operation cannot outlive the orphan lease.

Please fix this vulnerability. If you propose a fix, make it concise and minimal.

_{React 👍 / 👎 to tune Strix for this repo. A repo collaborator (or the PR author) can resolve this thread to dismiss the finding.}

[sanastasiou]

Good catch — this is a real race and I've taken the suggested fix. Pushed in 7d96b38.

I verified both premises in the source before accepting rather than taking them on trust:

• MemoryEngine._write_operation_progress (memory_engine.py:3579) does SET result_metadata = ... , updated_at = now(). Confirmed: the heartbeat lands on updated_at, never on claimed_at.
• _wall_timeout_for (poller.py:65) returns a timeout only for _RETAIN_OP_TYPES and None for everything else. Confirmed: consolidation is genuinely unbounded, so it can outlive a 24 h lease legitimately.

So the sweep now requires both timestamps to be stale — a job that is merely slow is never touched, only one that is old and silent.

One detail worth surfacing: my existing test_expired_lease_from_departed_worker_is_reclaimed started failing with this change, and that turned out to be the test being unfaithful rather than the fix being wrong. It inserted a stale claimed_at but let updated_at default to now() — so under the new predicate it correctly reclaimed nothing. Real abandoned rows have both stale: the two production corpses that motivated this PR had created == updated == 2026-07-05T14:31:37, never touched again, because no progress heartbeat ever ran. The fixture now sets both, which is what the wild actually looks like.

Added test_slow_but_live_job_with_fresh_heartbeat_is_not_reclaimed to pin the race directly: 50-day-old claim, heartbeat seconds ago, different worker → must not be reclaimed. tests/test_worker.py: 111 passed, ruff clean.

On the two adjacent points in the report — unfenced terminal writes (_mark_completed matching operation_id + status with no worker_id guard) and consolidation having no wall-clock ceiling — I agree with both, but they're pre-existing and independent of this change, so I've left them out to keep the diff reviewable. Happy to fold either into this PR or open separate ones, whichever maintainers prefer.

[koriyoshi2041]

One small correctness issue in the diagnostic receipt: PostgreSQL UPDATE ... RETURNING exposes the updated tuple. Because this statement sets worker_id = NULL before RETURNING operation_id, worker_id, operation_type, every returned worker_id is NULL, so owners = sorted({str(r["worker_id"]) ...}) will log owners: None rather than the departed container IDs.

The reclaim behavior itself still looks sound, but this loses the exact operator signal the warning promises. A regression could assert that caplog contains dead-container-abc123. One repair is a candidate CTE that selects/locks the stale rows and preserves their old owner IDs, followed by UPDATE ... FROM candidates ... RETURNING candidates.worker_id; alternatively, return only fields whose post-update values are intended.

[sanastasiou]

Good catch — confirmed and fixed in ced4ee0.

You're right about the mechanism: RETURNING yields the post-update tuple, and the same statement sets worker_id = NULL, so the receipt could never have named an owner. Reproduced it against the old code before changing anything — the log line was literally:

Worker live-worker reclaimed 1 operation(s) abandoned by 1 departed worker(s)
in schema None (lease 86400s; owners: None). Set HINDSIGHT_API_WORKER_ID ...

which is the worst version of this bug: accurate, useless, and indistinguishable from the sweep having matched a row with a genuinely NULL owner.

Took the candidate-CTE option you suggested:

WITH candidates AS (
    SELECT operation_id, worker_id
    FROM {table}
    WHERE status = 'processing'
      ...
    FOR UPDATE SKIP LOCKED
)
UPDATE {table} AS t
SET status = 'pending', worker_id = NULL, claimed_at = NULL, updated_at = now()
FROM candidates c
WHERE t.operation_id = c.operation_id
RETURNING c.operation_id, c.worker_id AS previous_worker_id, t.operation_type

Predicates and reclaim behaviour are unchanged; only the reported owner moves.

One addition beyond the repair: the CTE takes FOR UPDATE SKIP LOCKED. This runs on the startup path, so a rolling restart can have two workers sweeping at once — without it one blocks on the other's row locks for the duration of that transaction. With SKIP LOCKED they reclaim disjoint sets and neither waits, and a row locked by a live worker is skipped, which is the reading we want anyway.

Regression test is the one you described — asserts dead-container-abc123 appears in the warning and that owners: None does not. I mutation-checked it by reverting the SQL to the old RETURNING worker_id; the test fails there and passes on the fix, so it's actually pinning the behaviour rather than the implementation. Full worker suite: 112 passed (2 unrelated setup errors from sentence-transformers missing in my env).

[koriyoshi2041]

Re-checked the diagnostic repair at ced4ee0: the candidate CTE preserves the pre-update owner, keeps the original stale-row predicates, and FOR UPDATE SKIP LOCKED makes concurrent startup sweeps select disjoint rows. The regression now observes the operator-facing contract directly (dead-container-abc123 present, owners: None absent), so the issue I raised is resolved at this head. I did not re-evaluate the broader lease/fencing design in this follow-up.