PiCloud/HANDBACK.md

# v1.1.9 — HANDBACK

Branch: `feat/v1.1.9-queues-invoke` (cut from `main` at `b9e002a`).
Workspace: `picloud@1.1.9`. Dashboard: `0.15.0`. SDK schema: `1.10`.
**Pure additive — no upgrade-order constraint.**

## 1. Scope coverage

| Brief item | Status | Where |
|---|---|---|
| `queue::*` SDK (`enqueue` / `depth` / `depth_pending`) | ✅ | `crates/executor-core/src/sdk/queue.rs`, `crates/shared/src/queue.rs`, `crates/manager-core/src/queue_service.rs` |
| `queue:receive` trigger kind | ✅ | `TriggerKind::Queue`, `TriggerDetails::Queue`, `queue_trigger_details` |
| One-consumer-per-queue enforcement | ✅ (API-layer via `pg_advisory_xact_lock`) | `crates/manager-core/src/trigger_repo.rs::create_queue_trigger` |
| Dispatcher queue arm (FOR UPDATE SKIP LOCKED) | ✅ | `crates/manager-core/src/dispatcher.rs::tick_queue_arm` + `dispatch_one_queue` |
| Visibility-timeout reclaim task | ✅ | `crates/manager-core/src/dispatcher.rs::spawn` reclaim block + `QueueRepo::reclaim_visibility_timeouts` |
| Auto-ack / auto-nack / dead-letter | ✅ | `QueueRepo::ack` / `nack` / `dead_letter` + `handle_queue_failure` |
| Dead-letter fan-out for queue | ✅ (free — existing `fan_out_dead_letter` reads `source = "queue"`) | `dispatcher.rs::handle_failure` already wired |
| `invoke()` (sync) | ✅ | `crates/executor-core/src/sdk/invoke.rs` |
| `invoke_async()` + `OutboxSourceKind::Invoke` arm | ✅ | `sdk/invoke.rs::invoke_async` + `dispatcher::build_invoke_request` |
| Cross-app invoke rejected | ✅ | `InvokeService::resolve` → `InvokeError::CrossApp` |
| Depth bound shared with trigger fan-out | ✅ | `Limits::trigger_depth_max`, synced from `TriggerConfig::max_trigger_depth` |
| `retry::*` SDK (Policy + run + on_codes) | ✅ (with `retry::run` rename — see §7) | `crates/executor-core/src/sdk/retry.rs` |
| Migrations 0034 + 0035 | ✅ | as named |
| Admin HTTP — `/triggers/queue` + `/queues` + `/queues/{name}` | ✅ | `crates/manager-core/src/triggers_api.rs` + `queues_api.rs` |
| Dashboard Queues tab + Triggers form + 0.15.0 | ✅ | `dashboard/src/routes/apps/[slug]/queues/*` + extended `+page.svelte` + `package.json` |
| Schema snapshot re-blessed | ✅ | `crates/manager-core/tests/expected_schema.txt` |
| CHANGELOG | ✅ | this branch |
| Version bumps (1.1.8 → 1.1.9, SDK 1.9 → 1.10) | ✅ | `Cargo.toml`, `crates/shared/src/version.rs` |
| F1 — integration test density | ✅ | 4 new test binaries: queue_e2e (4) + invoke_e2e (4) + retry_e2e (3) + migration_queue_messages (4) + 50+ inline unit tests |
| F2 — schema snapshot BLESS=1 in same branch | ✅ | committed |
| F3 — literal fmt/clippy output | ✅ | §8 |
| F4 — TIMING_FLAT_DUMMY_HASH dedup | ✅ | `crates/manager-core/src/auth_api.rs::login` references `crate::auth::TIMING_FLAT_DUMMY_HASH` |
| F5 — clippy without cold cache | ✅ | incremental cache used |

## 2. Queue dispatcher design notes

**The queue table IS the outbox for queue semantics.** No
double-buffering through `outbox.source_kind = 'queue'`. The
dispatcher's `tick_queue_arm` lists active consumers
(`triggers.list_active_queue_consumers()` — joins `triggers` +
`queue_trigger_details` for enabled `kind = 'queue'` rows) and runs
one atomic claim per `(app_id, queue_name)` per tick.

Claim shape (single round trip):

```sql
UPDATE queue_messages
SET claim_token = $3, claimed_at = NOW(), attempt = attempt + 1
WHERE id = (
    SELECT id FROM queue_messages
    WHERE app_id = $1 AND queue_name = $2
      AND claim_token IS NULL
      AND (deliver_after IS NULL OR deliver_after <= NOW())
    ORDER BY enqueued_at
    FOR UPDATE SKIP LOCKED
    LIMIT 1
)
RETURNING id, app_id, queue_name, payload, enqueued_at, attempt, max_attempts
```

`SKIP LOCKED` keeps concurrent dispatchers (cluster mode v1.3+) safe.
The `claim_token` (fresh `Uuid::new_v4()` per claim) is the lease
guarantee: ack and nack both filter `WHERE id = $1 AND claim_token =
$2`, so a stale dispatcher whose visibility-timeout expired can't
accidentally delete or re-defer a message that's been re-claimed.

**Ack** (handler returned successfully):
`DELETE FROM queue_messages WHERE id = $1 AND claim_token = $2`.

**Nack** (handler threw, `attempt < max_attempts`):
`UPDATE … SET claim_token = NULL, claimed_at = NULL, deliver_after =
NOW() + retry_delay`. Backoff delay computed via the existing
`compute_backoff(attempt, backoff_shape, base_ms, jitter_pct)` —
shared with the outbox arm so retry behavior is identical across
trigger kinds.

**Dead-letter** (handler threw, `attempt >= max_attempts`): single
transaction — `INSERT INTO dead_letters` with `source = "queue"`,
`op = "receive"`, the original payload wrapped in a shape mirroring
`TriggerEvent::Queue` (so the existing `fan_out_dead_letter` path on
the outbox arm can fire registered `dead_letter` triggers off the new
row without any special-casing), then `DELETE FROM queue_messages`.

**Visibility-timeout reclaim**: separate `tokio::spawn` task, ticks
every `PICLOUD_QUEUE_RECLAIM_INTERVAL_MS` (default 30 000 ms). UPDATE
joins `queue_messages` + `triggers` + `queue_trigger_details` and
clears claims older than the per-queue `visibility_timeout_secs`. A
crashed consumer thus loses its lease and the message becomes
claimable again. **Verified** by `queue_e2e::queue_visibility_timeout_reclaim`
(inserts a row with `claimed_at = NOW() - 60s`, `visibility_timeout =
5s`; polls until either the dispatcher re-claims or the claim
clears).

## 3. `invoke()` re-entrancy notes

**Engine sharing** via `Engine::self_weak: OnceLock<Weak<Engine>>`.
The picloud binary:

```rust
let engine = Arc::new(Engine::new(engine_limits, services));
engine.set_self_weak(Arc::downgrade(&engine));
```

`Engine::execute_ast` reads `self.self_arc()` (which upgrades the
`Weak`) and threads `Option<Arc<Engine>>` through
`sdk::register_all(...)` to the invoke bridge.

**`Weak` is the right choice** — strong would make the Engine Arc
cycle itself, leaking on drop. Cycle stays broken; the invoke bridge
just gets `None` if the engine has been dropped (which only happens
on shutdown).

**Re-entry path** (inside `invoke.rs::invoke_blocking`):

1. Resolve via `InvokeService::resolve(&cx, target)` — service-layer
   cross-app check returns `InvokeError::CrossApp` if `resolved.app_id
   != cx.app_id`.
2. Depth check **before** resolve — `cx.trigger_depth + 1 >
   limits.trigger_depth_max` throws immediately (no wasted DB
   round-trip).
3. Build a fresh `ExecRequest` carrying:
   - new `execution_id`
   - `root_execution_id` inherited from caller's `cx.root_execution_id`
   - `trigger_depth = cx.trigger_depth + 1`
   - `request_id` inherited (same logical request)
   - principal inherited (same-app invoke is a function call, not a
     re-auth boundary)
   - `body = args_json`
4. Call `self_engine.execute(&resolved.source, req)` — synchronous,
   on the SAME `spawn_blocking` thread. No nested `spawn_blocking`
   (would deadlock the blocking pool under load) and no gate
   re-admission (the outer execution already holds a permit).
5. Return `json_to_dynamic(resp.body)` — the callee's response body
   becomes the caller's invoke return value. Status code + headers
   are dropped (the function-call mental model is "return value", not
   "HTTP response").

**Cross-app rejection point**: verified at three layers — repo
returns the script row, service compares `resolved.app_id` to
`cx.app_id`, and the dispatcher's `build_invoke_request` re-checks
when firing an `invoke_async` outbox row (defense in depth — the
service already enforced same-app at enqueue time, but a hand-edited
row should still be rejected).

**Depth-bound** integration tested by `invoke_e2e::invoke_depth_limit_exceeds_cleanly`
— recursive script propagates the throw all the way up; outer
caller's `try/catch` surfaces "invoke: depth limit exceeded (max 8)".

## 4. `retry::*` notes

**Closure passing** via Rhai's `FnPtr`. `retry::run(policy, fn_ptr)`
is registered with `NativeCallContext` so the bridge can call
`fn_ptr.call_within_context(ctx, ())` in a loop. The closure shares
the caller's engine + cx — if the closure itself calls `invoke()`,
the inner call goes through the invoke bridge (fresh cx with
`trigger_depth + 1`), retry doesn't see or interfere with that.

**Sleep mechanics**: `tokio::time::sleep(delay)` via
`TokioHandle::block_on`. We're already inside the caller's
`spawn_blocking` thread, so blocking the thread on a tokio sleep is
fine — the runtime's worker pool isn't being held.

**Policy clamping** at construction (`retry::policy(opts)`):

- `max_attempts ∈ [1, 20]` — saturating clamp
- `base_ms ∈ [1, 60_000]` — saturating clamp
- `jitter_pct ∈ [0, 100]` — saturating clamp
- `backoff ∈ {"exponential" | "linear" | "constant"}` — bogus values
  surface a clear error
- `on_codes` defaults to empty (retry every throw); when non-empty,
  filters by substring match

**Jitter**: deterministic via `DefaultHasher` over `(span, raw)` —
random distribution doesn't matter for retry; bounded ± is all we
need. Avoids pulling `rand` into the hot path.

## 5. Dashboard notes

- New routes: `/apps/[slug]/queues` (list) and
  `/apps/[slug]/queues/[name]` (drill-down). Both use Svelte 5 runes
  (`$state`, `$derived`, `$effect`) — matches the existing app
  detail page's idioms.
- Triggers tab gains a "Queue:receive trigger (v1.1.9)" form below the
  Email form. Same `submitCreate*` pattern as the other forms; clears
  state on success and re-loads the trigger list.
- App-level nav gains a "Queues" link between Files and Dead letters,
  guarded by `canAdmin` (same gate as the other operational tabs).
- TypeScript types: `TriggerKind` admits `'queue'`, `TriggerDetails`
  union admits the `{ kind: 'queue', queue_name, visibility_timeout_secs,
  last_fired_at }` shape. `CreateQueueTriggerInput`, `QueueSummary`,
  `QueueConsumer`, `QueueDetail` exported alongside.
- `npm run check`: my changes typecheck clean. (The 149 pre-existing
  errors are all in `tests/e2e/**/*.spec.ts` about `@playwright/test`
  not being installed — unchanged from main.)
- `dashboard/package.json` bumped to `0.15.0`.

## 6. F1–F5 implementation notes

**F1 — integration test density.** Four new DB-gated test binaries
(all skip cleanly when `DATABASE_URL` is unset, mirroring the
existing `dispatcher_e2e.rs` pattern):

- `crates/picloud/tests/queue_e2e.rs` (4 tests, 33s runtime):
  - `queue_receive_acks_on_success`
  - `queue_receive_dead_letters_after_max_attempts`
  - `queue_one_consumer_per_queue_rejected`
  - `queue_visibility_timeout_reclaim`
- `crates/picloud/tests/invoke_e2e.rs` (4 tests, 2s):
  - `invoke_by_name_same_app_returns_value`
  - `invoke_cross_app_rejects`
  - `invoke_depth_limit_exceeds_cleanly`
  - `invoke_async_enqueues_outbox_row`
- `crates/picloud/tests/retry_e2e.rs` (3 tests, 2.5s):
  - `retry_run_eventually_succeeds_inside_http_handler`
  - `retry_run_surfaces_last_error_after_max_attempts`
  - `retry_on_codes_filters_unmatched_errors`
- `crates/manager-core/tests/migration_queue_messages.rs` (4 tests):
  - `queue_messages_table_exists_with_expected_columns`
  - `queue_trigger_details_table_exists`
  - `queue_widens_trigger_kind_and_outbox_source_kind`
  - `queue_messages_dispatch_index_is_partial`

Plus 50+ net new inline unit tests across the affected crates. All
pass against the docker-compose postgres on `localhost:15432`. See
§8 for the literal `cargo test` output.

**F2 — schema snapshot `BLESS=1` part of the gate.** Re-blessed via:

```sh
DATABASE_URL=postgres://picloud:picloud@localhost:15432/picloud \
PICLOUD_ADMIN_USERNAME=dev PICLOUD_ADMIN_PASSWORD=dev \
BLESS=1 cargo test -p picloud-manager-core --test schema_snapshot \
  -- --include-ignored
```

Delta committed in `chore(v1.1.9): re-bless schema snapshot (0034 + 0035 delta)`.
Matches the plan exactly: two new tables, three new partial indexes,
widened `triggers.kind` + `outbox.source_kind` CHECKs, no unrelated
drift.

**F3 — fmt + clippy attestation as literal output.** See §8.

**F4 — `TIMING_FLAT_DUMMY_HASH` dedup.** `crates/manager-core/src/auth_api.rs::login`
now reads `crate::auth::TIMING_FLAT_DUMMY_HASH.to_string()` on the
bad-email branch. Verification:

```sh
$ grep -rn 'argon2id\$v=19\$m=19456,t=2,p=1\$dGltaW5n' crates/
crates/manager-core/src/auth.rs:36:    "$argon2id$v=19$m=19456,t=2,p=1$dGltaW5nLWZsYXR0ZW4$Ux6dgPqgX1Mhg5fRgIeKZF3MWdYqJplKEz/cKLcSdks";
```

Exactly one hit — the const declaration in `auth.rs`.

**F5 — clippy cold-cache environmental constraint.** Used the
incremental cache (no `cargo clean`). Verified by checking the
`Checking <crate>` lines for test crates appear in the clippy output.
CI will do the cold-cache attestation on its dedicated runner.

## 7. Decisions beyond the brief / deviations flagged

**D1 — `retry::with` → `retry::run` (script-side name).** The brief
specified `retry::with(policy, closure)`. Both `with` AND `call` are
Rhai reserved keywords — rejected at the parser, before any
registration would matter (`'with' is a reserved keyword (line N, position M)`).
Shipped as `retry::run(policy, closure)`. Documented in the docstring,
the SDK_VERSION 1.10 changelog comment, and the dashboard form copy.

**D2 — Trait move skipped (was plan §5).** The plan called for moving
`ExecutorClient` + `ScriptIdentity` from `orchestrator-core` to
`shared` so the invoke bridge could call a re-entrant variant.
Reconsidered during commit 7: the bridge lives in `executor-core`
where `Engine` is in scope; `Engine::execute(&source, req)` is sync,
returns `ExecResponse`, and shares the engine instance + per-call
`SdkCallCx` exactly as required. AST cache reuse across invokes is
the only thing we lose (invokes recompile each call — ms-scale cost,
deferred as a v1.2 optimization). Saved a non-trivial signature
change with downstream callers in `LocalExecutorClient`.

**D3 — Retry columns on parent triggers row only (was plan §1).** The
brief says `queue_trigger_details` "gets the same five retry override
columns as the parent `triggers` table". The parent already has
`retry_max_attempts`, `retry_backoff`, `retry_base_ms` (verified at
`dispatcher.rs:246-249`). Duplicating them on the detail row would
split the source of truth. Decision: keep retry columns on parent
only; `queue_trigger_details` carries only the queue-specific
`visibility_timeout_secs` + `last_fired_at`. Surfaced here per
discipline reminder #2 ("flag, don't reinterpret").

**D4 — Trigger-depth bound mirrored on `Limits` (was plan §5).** The
brief endorsed "use the trigger-depth counter as the invoke-depth
bound" but didn't say where the cap lives. Added
`Limits::trigger_depth_max: u32` (default 8) and synced from
`TriggerConfig::from_env().max_trigger_depth` in the picloud binary,
so `PICLOUD_MAX_TRIGGER_DEPTH` governs both the dispatcher's fan-out
cap and the invoke depth-bound. Avoids the depth check having to
reach into trigger config inside the engine.

**D5 — One-consumer-per-queue via advisory lock (was plan §1).** A
partial unique index across `triggers.app_id` and
`queue_trigger_details.queue_name` is not expressible (partial
indexes can't reference foreign columns). Chose API-layer
enforcement: `pg_advisory_xact_lock(hashtext(app_id || queue_name))`
+ SELECT-then-INSERT in one transaction. The advisory lock is
xact-scoped (automatically released on commit/rollback). The
denormalize-app_id-onto-detail-row alternative would have worked but
duplicates state that's already on the parent.

**D6 — `invoke()` exposed globally (not `invoke::*`).** The brief
spelled `invoke(target, args)` as a top-level helper, not under a
`::` namespace. Registered via `engine.register_global_module(...)`
so scripts write `invoke("worker")` rather than `invoke::call("worker")`.

**D7 — `invoke_async` runs once.** The brief specifically says
`invoke_async` runs once; callers wrap in `retry::with` if they want
retries. Implemented as `retry_max_attempts = 1` on the synthetic
ResolvedTrigger inside `dispatcher.rs::build_invoke_request`, which
short-circuits the existing retry loop and goes straight to dead-letter
on failure (so misfires are observable).

## 8. Verification (literal output)

```sh
$ cargo fmt --all -- --check 2>&1 | tail -3
# no output (exit 0)

$ cargo clippy --workspace --all-targets --all-features -- -D warnings 2>&1 | tail -3
    Checking picloud v1.1.9 (/home/fabi/PiCloud/crates/picloud)
    Checking picloud-manager v1.1.9 (/home/fabi/PiCloud/crates/picloud-manager)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 12.42s
```

**F5 note**: ran clippy WITHOUT a `cargo clean` first (incremental
cache used). The `Checking …` lines for test crates appear in the
unfiltered output. CI will do the cold-cache attestation on its
dedicated runner.

**Workspace lib unit smoke** (per-crate, no `DATABASE_URL`):

```
shared:            34 passed
executor-core:    218 passed   (74 lib + 144 across the ten tests/sdk_*.rs binaries)
manager-core:     311 passed   (306 lib + 4 migration + 1 schema_snapshot)
orchestrator-core: 74 passed
```

**DB-gated E2E** (against `docker compose up -d postgres`,
`DATABASE_URL=postgres://picloud:picloud@localhost:15432/picloud`):

```
picloud queue_e2e:       4 passed in 33.21s
picloud invoke_e2e:      4 passed in 3.38s
picloud retry_e2e:       3 passed in 1.65s
picloud dispatcher_e2e:  9 passed in 32.70s
picloud api:             8 passed in 3.85s
picloud authz:           4 passed in 2.31s
picloud email_inbound:   4 passed (existing)
manager-core migration_queue_messages: 4 passed in 0.12s
manager-core schema_snapshot: 1 passed in 0.28s
```

All pass. Total across all binaries (lib + integration + DB-gated):
~660 tests.

**F4 verification**:

```sh
$ grep -rn 'argon2id\$v=19\$m=19456,t=2,p=1\$dGltaW5n' crates/
crates/manager-core/src/auth.rs:36:    "$argon2id$v=19$m=19456,t=2,p=1$dGltaW5nLWZsYXR0ZW4$Ux6dgPqgX1Mhg5fRgIeKZF3MWdYqJplKEz/cKLcSdks";
```

Exactly one hit.

**Dashboard typecheck**:

```sh
$ cd dashboard && npx svelte-check --tsconfig ./tsconfig.json 2>&1 | grep -E "queues/|api\.ts" | head -10
1780778502345 ERROR "tests/e2e/fixtures/api.ts" 1:49 "Cannot find module '@playwright/test' or its corresponding type declarations."
```

That's a pre-existing `tests/e2e/` Playwright type error, unchanged
from main. My queue-related code typechecks clean.

## 9. Open questions for the reviewer

1. **`retry::run` rename** (D1): is `run` an acceptable script-side
   name? Alternatives that compile: `retry::do_attempts`,
   `retry::attempt`. `run` reads cleanest in practice but is
   slightly less specific than `with`.
2. **`invoke()` path-resolution method**: defaulted to POST→GET
   fallback. A future surface bump could let scripts pass a method
   (`invoke("/api/foo", "GET", #{})`) — flagged as v1.2 follow-up.
3. **`ctx.trigger_depth` not in `ctx`**: noted as a v1.2 follow-up
   inside `sdk_invoke.rs::invoke_callee_sees_incremented_depth`. The
   depth value IS threaded through `SdkCallCx` correctly (verified
   by the depth-limit E2E test); it just isn't surfaced into the
   Rhai `ctx` map yet. Trivial addition when v1.2 lands.
4. **Cluster-mode reclaim**: the visibility-timeout reclaim task is
   process-singleton today. Cluster mode (v1.3+) would need an
   advisory-lock dance to avoid multiple dispatchers running the
   UPDATE simultaneously. Out of scope per the brief.

## 10. Latent findings

**L1 — Pre-existing clippy lints surfaced during the F3 attestation.**
Six lints — two on new v1.1.9 code (`sdk/invoke.rs::_LIMITS_IS_COPY`
items-after-test-module, `picloud/lib.rs::engine_limits`
field-reassign-with-default), four others on new v1.1.9 SDK paths
(`retry.rs`, `queue.rs`, `dispatcher.rs`). All fixed alongside the
new code in commit `chore(v1.1.9): clippy clean — workspace -D warnings`
so the gate is green now. No carry-forward latent findings from main.

**L2 — Dashboard pre-existing Playwright errors (149)** in
`tests/e2e/**/*.spec.ts` — `Cannot find module '@playwright/test'`.
Pre-existing; my changes don't touch the e2e folder.

## 11. Deferred items

**Not deferred:** `retry::*` shipped as `retry::run` (with the rename
deviation flagged in §7). The full v1.1.9 scope landed.

**Standing v1.2+ deferred list** (no change from the brief):

- Cross-app `invoke()`
- Queue priorities
- Cron-style scheduled enqueues
- Queue purge / delete / requeue admin UI
- Cluster-mode `invoke()` (orchestrator → executor RPC)
- Distributed retry / sagas / compensations
- Closures captured across `invoke()` boundaries (rejected at the
  bridge — see §12)
- `ctx.trigger_depth` surface in the Rhai `ctx` map (v1.2 follow-up)
- AST cache reuse across `invoke()` calls (deferred D2 optimization)
- `invoke()` method-aware route resolution (currently POST→GET fallback)

## 12. Known limitations

- **No closures across `invoke()` boundaries.** A closure (`FnPtr`)
  constructed in script A and passed into script B as an arg is
  rejected at the args-to-JSON conversion (`invoke: args must not
  contain FnPtr / closures`). Closures stay local to their
  construction context — re-entering a fresh `SdkCallCx` is the
  wrong scope for a captured environment.
- **`invoke()` is in-process only.** Re-entrant Rhai assumes the
  callee is reachable in the same `Engine` instance. Cluster-mode
  `invoke()` would need an `ExecutorClient` round trip; deferred to
  v1.3+.
- **`invoke_async` does not retry.** One shot. Callers who want
  retries wrap the call site in `retry::run(policy, ...)`.
- **Queue depth-pending is approximate in the drill-down view.** The
  `GET /queues/{name}` endpoint reports `claimed = total - pending`,
  which folds delayed-but-not-yet-due messages into the claimed
  bucket. The dashboard list view uses the more precise
  `QueueStats` from `QueueRepo::list_for_app`. (Cosmetic; the data
  is accurate just slightly differently shaped.)
- **One dispatcher per process.** The queue arm + reclaim task run
  in the single MVP dispatcher. Cluster-mode multi-dispatcher
  coordination is v1.3+.
- **No mutating queue admin endpoints.** Purge / requeue /
  delete-message stays at v1.2. The dashboard surfaces the queue
  state read-only.