Keep legacy queue files when DB is in-memory fallback

If opening message-lifecycle.db fails, getLifecycleDb transparently falls back to :memory:; importLegacyFileQueue still proceeds and then deletes each legacy delivery-queue/*.json file. In that failure mode (for example, transient SQLite open failures), startup migrates pending deliveries into non-durable memory and removes the only durable source, so those unsent messages are lost after the next restart.

Useful? React with 👍 / 👎.

Already addressed — importLegacyFileQueue checks isLifecycleDbInMemory(stateDir) at line 359 and only deletes legacy .json files when the DB is persistent (if (!inMemory) at line 414). When the DB falls back to in-memory, legacy files are preserved as the durable source.

Prune delivered outbox rows after acknowledgement

ackDelivery now marks rows as delivered instead of deleting them, but there is no caller that runs pruneOutbox, so successful deliveries accumulate indefinitely in message_outbox. On busy/long-lived gateways this causes unbounded DB growth and eventually degrades write and recovery performance; either remove rows on ack or wire periodic pruning.

Useful? React with 👍 / 👎.

Periodic pruning is wired up in the write-ahead PR (#30009) which calls pruneOutbox on a schedule. Keeping rows on ack is intentional here so the write-ahead layer can track delivery stats per-turn before pruning.

dispatchKind parameter defined but never set anywhere in codebase - all entries will have NULL value. Consider adding test coverage for the dispatch_kind filtering behavior once this feature is fully implemented.

This is a comment left during a code review.
Path: src/infra/outbound/delivery-queue.ts
Line: 45

Comment:
`dispatchKind` parameter defined but never set anywhere in codebase - all entries will have NULL value. Consider adding test coverage for the `dispatch_kind` filtering behavior once this feature is fully implemented.

How can I resolve this? If you propose a fix, please make it concise.

dispatch_kind is set in the write-ahead PR (#30009) which populates it when enqueueing outbox entries. The column is defined here so the schema is ready; test coverage for filtering will land with that PR.

Reuse in-memory fallback for all subsequent lifecycle DB opens

When opening message-lifecycle.db fails, the fallback is cached only under memory:${dbPath} and not under the canonical dbPath key, so a later getLifecycleDb call can silently switch to a newly opened file-backed DB once the filesystem issue clears. That mid-process backend switch strands any outbox rows written to the in-memory DB and can drop pending deliveries after restart; it also leaves isLifecycleDbInMemory true because the memory key remains cached. Keep one stable cache key per stateDir (or check the memory key before retrying disk open) to avoid losing queued data.

Useful? React with 👍 / 👎.

Fixed — getLifecycleDb() now checks memory:${dbPath} in the cache before attempting to open the disk DB. Once a path falls back to in-memory, all subsequent calls for that stateDir return the in-memory DB for the rest of the process lifetime. See commit 830c6f8.

Restrict failDelivery to non-terminal outbox rows

failDelivery reads and updates by id only, so it can overwrite rows that were already terminalized (including delivered). Startup recovery is kicked off in a detached async task and can overlap with live delivery, so if one path ackDeliverys a row and the other path later calls failDelivery for that same row, this code flips it back to retryable and it gets re-sent on a later recovery despite already being delivered. Guard the select/update with a status predicate (only queued/failed_retryable) so terminal rows cannot be resurrected.

Useful? React with 👍 / 👎.

Fixed — the failDelivery SELECT now guards with AND status IN ('queued','failed_retryable') so it is a no-op if the row is already terminal. This prevents the recovery worker from resurrecting rows that the live path already acked/failed.

pruneOutbox(ageMs) will delete all terminal rows if called with ageMs <= 0 (cutoff becomes now or in the future). Since this is an exported helper, consider validating ageMs (finite, >0) and no-op/log when invalid to avoid accidental mass deletion via misconfiguration or call-site bugs.

Fixed in d9f04d1. pruneOutbox now guards against invalid ageMs (<=0 or non-finite) with an early return and log message.

ensureLifecycleSchema() unconditionally drops idx_message_outbox_idem and then recreates it, which means every process start will perform a DROP+CREATE even when no migration is needed. This can add avoidable lock contention and startup cost; consider only dropping when you detect an old/incorrect index definition (or gate the DROP behind a one-time schema version).

Fixed in d9f04d1. Removed the unconditional DROP INDEX IF EXISTS — CREATE UNIQUE INDEX IF NOT EXISTS is sufficient and the drop/recreate cycle briefly removes the unique constraint.

This test suite now opens a cached SQLite DB via getLifecycleDb(tmpDir), but the afterEach only deletes tmpDir. Because the DB handle is cached globally, this can leak file descriptors and may prevent tmpDir deletion on platforms like Windows. Consider calling clearLifecycleDbCacheForTest() (or otherwise closing the DB) in afterEach when using getLifecycleDb in tests.

Fixed in d9f04d1. Added clearLifecycleDbCacheForTest() in afterEach before rmSync so cached DB handles are released.

The comment says recovery defers fresh entries for 5s to avoid racing with the direct delivery path, but the SQLite outbox implementation shown here enqueues with next_attempt_at=now and loadPendingDeliveries() has no age cutoff. Either implement the stated deferral (e.g., ignore queued_at newer than a threshold on startup) or adjust the comment/test helper so it matches actual behavior.

Fixed in d9f04d1. Updated the comment to describe the actual next_attempt_at mechanism instead of the stale "5s delay" reference.

getLifecycleDb() enables WAL/NORMAL but does not configure a busy timeout. Without PRAGMA busy_timeout, concurrent access can fail immediately with SQLITE_BUSY instead of waiting briefly; consider setting a small busy_timeout (consistent with other SQLite usage in the repo) to reduce spurious lock errors under load.

Fixed in d9f04d1. Added PRAGMA busy_timeout=5000; after the WAL/NORMAL pragmas to prevent SQLITE_BUSY during concurrent access.

getLifecycleDb() silently falls back to an in-memory database on open errors. For an outbox intended to provide durable delivery across restarts, this fallback can cause message loss after a crash/restart while appearing to work. Consider failing fast (or at least logging at warn/error and disabling outbox/recovery) when the persistent DB cannot be opened, instead of switching to :memory: in production paths.

Intentional design tradeoff documented in the code comment at lines 19-20. An in-memory outbox that loses data on crash is strictly better than crashing the gateway entirely. The isLifecycleDbInMemory() function is exposed for callers that need to know the fallback state. Additionally, logVerbose emits a warning when the fallback triggers.

isEntryEligibleForRecoveryRetry() computes backoff using computeBackoffMs(entry.retryCount + 1), but failDelivery() schedules next_attempt_at using computeBackoffMs(attempt_count). This off-by-one makes recovery more conservative than the persisted schedule (e.g., retryCount=1 waits 25s instead of 5s) and can leave entries deferred during the one-time startup recovery. Align the backoff calculation with attempt_count/next_attempt_at (or simply use next_attempt_at as the source of truth).

Verified not a bug. The math is consistent:

1. failDelivery increments attempt_count to nextCount and writes next_attempt_at = now + computeBackoffMs(nextCount)
2. isEntryEligibleForRecoveryRetry reads entry.retryCount (= stored attempt_count = nextCount) and computes computeBackoffMs(entry.retryCount) — same backoff level

The +1 in isEntryEligibleForRecoveryRetry at line 342 was from an earlier version. On the current branch, this reads computeBackoffMs(entry.retryCount) directly (the stored incremented value), matching what failDelivery used to compute next_attempt_at.

failDelivery(): when classifying an error as permanent, the UPDATE does not increment attempt_count or set last_attempt_at. This makes the outbox row look like it never had an attempt despite being terminally failed, and can skew metrics/debugging. Consider incrementing attempt_count and recording last_attempt_at consistently for all failure paths (permanent, retryable, terminal-by-max-retries).

Fixed in d9f04d1. The permanent-error path in failDelivery now also sets attempt_count and last_attempt_at for observability.

recoverPendingDeliveries() logs that remaining entries are "deferred to next tick", but this recovery is invoked only once on gateway startup (no periodic rerun in this PR). The message is misleading; consider changing it back to "next restart" or "next recovery run" (or scheduling a follow-up run if "tick" is intended).

Fixed in d9f04d1. Changed "deferred to next tick" to "deferred to next recovery pass" which is accurate in both startup-only and periodic recovery contexts.

Guard moveToFailed from overwriting terminal outbox rows

moveToFailed updates by id only, so it can overwrite rows that another path already terminalized as delivered. Startup recovery is launched in a detached task from startGatewayServer and can overlap live delivery, so a concurrent ackDelivery followed by this update can flip a successfully delivered row back to failed_terminal, corrupting lifecycle state and downstream delivery accounting; constrain this update to non-terminal statuses (for example queued/failed_retryable) like failDelivery does.

Useful? React with 👍 / 👎.

Fixed in fc01448. Added AND status IN ('queued','failed_retryable') predicate to the moveToFailed UPDATE, mirroring the guard in failDelivery. Also added a test that acks a row first and verifies moveToFailed is a no-op for already-delivered rows.