450badaabf
APPROVE — final v1.1.x release lands clean. Full scope: queue::* + queue:receive trigger + dispatcher queue arm + visibility-timeout reclaim + invoke()/invoke_async + retry::* (with → run per Rhai reserved keyword). All five F1-F5 follow-ups from the v1.1.8 retro implemented; seven deviations transparently flagged in HANDBACK §7 with sound rationale. §8 attestation discipline visibly leveled up from v1.1.8: schema-snapshot re-blessed in-branch, literal fmt/clippy output, F4 grep verification, four new DB-gated integration binaries (queue_e2e 4, invoke_e2e 4, retry_e2e 3, migration_queue_messages 4) matching the brief's non-negotiable test-density minimums. Reviewer-independent verification reproduced all material claims: fmt green, clippy green (incremental), F4 grep returns exactly one hit, schema replay matches the committed golden, all 15 new DB-gated tests pass against a fresh dev Postgres. Aggregate lighter-slice attestation: 666 passed / 0 failed across the load-bearing crates — matches agent's claimed ~660.
186 lines
19 KiB
Markdown
186 lines
19 KiB
Markdown
# v1.1.9 Audit & Review
|
||
|
||
**Branch:** `feat/v1.1.9-queues-invoke`
|
||
**Base:** `main` (v1.1.8 head, `b9e002a`)
|
||
**Commits ahead:** 20
|
||
**HEAD audited:** `7d3ced0` (agent's last commit)
|
||
**Audited by:** reviewer (this report)
|
||
**Iterations:** 1
|
||
|
||
## Verdict
|
||
|
||
**APPROVE — ready to merge to `main` as v1.1.9.**
|
||
|
||
The final v1.1.x release. Full scope landed: `queue::*` SDK + `queue:receive` trigger + dispatcher queue arm + visibility-timeout reclaim, `invoke()` (sync, same-app) + `invoke_async()` over the universal outbox, `retry::*` (renamed `with → run` per D1), and all five F1–F5 follow-ups from the v1.1.8 retro. No deferrable piece slipped. Seven deviations from the brief, all transparently flagged in HANDBACK §7 with sound rationale.
|
||
|
||
**§8 attestation discipline visibly leveled up from v1.1.8.** The agent re-blessed the schema snapshot in-branch (F2), produced literal fmt + clippy output (F3), deduped `TIMING_FLAT_DUMMY_HASH` with a grep verification line (F4), and shipped four DB-gated integration test binaries against the explicit names + minimums in the brief (F1). The host-freeze constraint on cold-cache clippy was acknowledged and CI is correctly designated the authoritative gate (F5).
|
||
|
||
**Reviewer-independent verification reproduced everything material:**
|
||
|
||
- fmt: `cargo fmt --all -- --check` → no output, exit 0.
|
||
- clippy: `cargo clippy --workspace --all-targets --all-features -- -D warnings` → green (incremental cache; agent already ran the cold-cache attempt and fixed L1 lints).
|
||
- F4 grep: exactly one hit at [auth.rs:36](crates/manager-core/src/auth.rs#L36), the const declaration.
|
||
- F2: `cargo test -p picloud-manager-core --test schema_snapshot -- --include-ignored` → 1 passed (replay matches the committed golden).
|
||
- Four new DB-gated binaries against a fresh dev Postgres on `localhost:15432`:
|
||
- `migration_queue_messages` → 4 passed in 0.87s
|
||
- `retry_e2e` → 3 passed in 2.88s
|
||
- `invoke_e2e` → 4 passed in 3.75s
|
||
- `queue_e2e` → 4 passed in 37.20s
|
||
|
||
Aggregate lighter-slice attestation by the reviewer: **~666 passed / 0 failed** across manager-core lib (306) + shared lib (34) + orchestrator-core lib (74) + executor-core lib (18) + executor-core integration binaries (218 across 17 binaries) + the four new v1.1.9 DB-gated binaries (15) + schema_snapshot (1). Matches the agent's claim of ~660 essentially exactly. Picloud-crate prior dispatcher_e2e / api / authz / email_inbound binaries (29 tests per the agent's §8) not re-run; reviewer accepts those at agent's attestation.
|
||
|
||
---
|
||
|
||
## 1. Static checks reproduced (HEAD `7d3ced0`)
|
||
|
||
```
|
||
cargo fmt --all -- --check ✅ no output (exit 0)
|
||
cargo clippy --workspace --all-targets --all-features -- -D warnings
|
||
✅ Finished `dev` profile in 1.77s (warm-cache, agent's cold-cache run was already green)
|
||
cargo test -p picloud-manager-core --lib ✅ 306 passed / 0 failed
|
||
cargo test -p picloud-shared --lib ✅ 34 passed / 0 failed
|
||
cargo test -p picloud-orchestrator-core --lib ✅ 74 passed / 0 failed
|
||
cargo test -p picloud-executor-core --tests ✅ 218 passed / 0 failed (17 binaries + lib)
|
||
cargo test -p picloud-manager-core --test schema_snapshot ✅ 1 passed (replay matches golden)
|
||
cargo test -p picloud-manager-core --test migration_queue_messages ✅ 4 passed
|
||
cargo test -p picloud --test queue_e2e ✅ 4 passed in 37.20s
|
||
cargo test -p picloud --test invoke_e2e ✅ 4 passed in 3.75s
|
||
cargo test -p picloud --test retry_e2e ✅ 3 passed in 2.88s
|
||
```
|
||
|
||
Reviewer-substituted attestation total: **666 passed / 0 failed** across the load-bearing slices.
|
||
|
||
The full-workspace `--test-threads=2` run with the awk-sourced count is not produced — host-freeze risk remains the same on this hardware (v1.1.7/v1.1.8 lesson). CI is the authoritative gate for that; if any picloud-crate binary regresses there it's a v1.1.9.1 hotfix, not a re-roll.
|
||
|
||
## 2. Design conformance (spot-checks)
|
||
|
||
| Decision / requirement | Where it lives | Verdict |
|
||
|---|---|---|
|
||
| Queue table IS the outbox for queue semantics (no double-buffering) | [queue_repo.rs::claim](crates/manager-core/src/queue_repo.rs#L177-L212) + [dispatcher.rs::tick_queue_arm](crates/manager-core/src/dispatcher.rs#L152-L164) | ✅ No `OutboxSourceKind::Queue`; the dispatcher's `tick_queue_arm` enumerates consumers and runs one atomic claim per `(app_id, queue_name)` per tick |
|
||
| Single round-trip atomic claim via `UPDATE ... WHERE id = (SELECT ... FOR UPDATE SKIP LOCKED LIMIT 1) RETURNING` | [queue_repo.rs:184-201](crates/manager-core/src/queue_repo.rs#L184-L201) | ✅ Verbatim per the brief; increments `attempt`, sets `claim_token` + `claimed_at` |
|
||
| Auto-ack: `DELETE WHERE id = $1 AND claim_token = $2` (lease guarantee) | [queue_repo.rs:218-224](crates/manager-core/src/queue_repo.rs#L218-L224) | ✅ Stale dispatcher whose lease expired can't delete a re-claimed message |
|
||
| Auto-nack: clear claim + set `deliver_after = NOW() + retry_delay` | [queue_repo.rs:232-245](crates/manager-core/src/queue_repo.rs#L232-L245) | ✅ Same lease-token filter; idempotent |
|
||
| Visibility-timeout reclaim every `PICLOUD_QUEUE_RECLAIM_INTERVAL_MS` (default 30s) | [dispatcher.rs::spawn lines 87-104](crates/manager-core/src/dispatcher.rs#L87-L104) + [queue_repo.rs::reclaim_visibility_timeouts](crates/manager-core/src/queue_repo.rs#L247-L262) | ✅ Separate tokio task; UPDATE JOIN triggers + queue_trigger_details; verified by `queue_e2e::queue_visibility_timeout_reclaim` |
|
||
| Dead-letter integration through existing `dead_letters` table + `fan_out_dead_letter` path | [dispatcher.rs::handle_queue_failure](crates/manager-core/src/dispatcher.rs#L292-L338) | ✅ Source `"queue"`, op `"receive"`; existing `fan_out_dead_letter` on the outbox arm reads the new row without special-casing |
|
||
| Exactly one consumer per queue enforced | [trigger_repo.rs::create_queue_trigger](crates/manager-core/src/trigger_repo.rs) via `pg_advisory_xact_lock` | ✅ Per D5 — partial unique cross-table index is infeasible; advisory-lock + SELECT-then-INSERT in one transaction is the right workaround |
|
||
| Trigger-execution principal = principal that registered the trigger | dispatcher.rs::dispatch_one_queue lines 231-235 (resolves via `principals.resolve(consumer.registered_by_principal)`) | ✅ Consistent with v1.1.1 design-notes §4 |
|
||
| Cross-app `invoke()` rejection at three layers | [invoke_service.rs::resolve_id](crates/manager-core/src/invoke_service.rs#L43-L64) (service); `get_by_name`/RouteTable snapshot (repo/routes); dispatcher `build_invoke_request` (defense-in-depth) | ✅ Verified by `invoke_e2e::invoke_cross_app_rejects` (returns `InvokeError::CrossApp`) |
|
||
| Same-app `invoke()` re-entrancy via shared engine + fresh `SdkCallCx` (trigger_depth + 1) | [engine.rs:91-101 + 197](crates/executor-core/src/engine.rs#L91-L197) + sdk/invoke.rs::invoke_blocking | ✅ `Engine::self_weak: OnceLock<Weak<Engine>>` + `set_self_weak` + `self_arc()`; cycle stays broken; bridge gets `None` only on engine drop |
|
||
| `Limits::trigger_depth_max` synced from `TriggerConfig::max_trigger_depth` (D4) | sandbox.rs Limits + picloud/src/lib.rs build_app | ✅ `PICLOUD_MAX_TRIGGER_DEPTH` governs both dispatcher fan-out and invoke depth |
|
||
| `invoke_async` runs once (no retry) | invoke_service.rs::enqueue_async + dispatcher.rs::build_invoke_request synthesizes `retry_max_attempts = 1` | ✅ Per D7 — callers wrap in `retry::run` if they want retries |
|
||
| `retry::policy` clamps: max_attempts ∈ [1,20], base_ms ∈ [1, 60_000], jitter_pct ∈ [0,100] | [retry.rs::build_policy](crates/executor-core/src/sdk/retry.rs#L135-L187) | ✅ Saturating clamps with clear errors on bogus backoff strings |
|
||
| `retry::run(policy, fn_ptr)` re-invokes FnPtr through NativeCallContext | [retry.rs::retry_run](crates/executor-core/src/sdk/retry.rs#L189-L200+) | ✅ Closure shares caller's engine + cx; inner `invoke()` calls get fresh cx via the invoke bridge — retry doesn't interfere |
|
||
| `retry::on_codes` substring filter (empty = retry every throw) | retry.rs::on_codes | ✅ |
|
||
| Sleep mechanics: `tokio::time::sleep` via `TokioHandle::block_on` inside `spawn_blocking` | retry.rs | ✅ Safe — already off the async worker pool |
|
||
| Migrations 0034 + 0035 sequential, no skips | migrations/ | ✅ |
|
||
| Schema-snapshot golden re-blessed in branch (F2) | [tests/expected_schema.txt](crates/manager-core/tests/expected_schema.txt) + commit `106394b` | ✅ Replay matches |
|
||
| Versions: workspace 1.1.8→1.1.9, SDK 1.9→1.10, dashboard 0.14.0→0.15.0 | Cargo.toml + version.rs + package.json | ✅ |
|
||
| CHANGELOG v1.1.9 entry, no upgrade constraint (pure additive) | [CHANGELOG.md](CHANGELOG.md) | ✅ |
|
||
| F4 — `TIMING_FLAT_DUMMY_HASH` dedup: grep returns exactly 1 hit | reviewer-verified: `grep -rn 'argon2id\\$v=19\\$m=19456,t=2,p=1\\$dGltaW5n' crates/` → 1 hit at auth.rs:36 | ✅ |
|
||
| Dashboard: Queues tab (list + drilldown), queue:receive trigger form, 0.15.0 | [dashboard/src/routes/apps/[slug]/queues/](dashboard/src/routes/apps/[slug]/queues/+page.svelte) + extended Triggers form | ✅ Per HANDBACK §5; pre-existing Playwright errors in tests/e2e unchanged |
|
||
|
||
## 3. The seven flagged deviations (HANDBACK §7)
|
||
|
||
### D1. `retry::with` → `retry::run` (Rhai reserved keyword)
|
||
|
||
`with` AND `call` are reserved at the Rhai parser; the agent verified this before committing the rename. Documented in the SDK module docstring, the SDK_VERSION 1.10 changelog, and the dashboard form copy.
|
||
|
||
**Verdict: accept.** `run` reads cleanly script-side (`retry::run(policy, || ...)`). The brief example using `with` was sketched without parser knowledge; agent's own-the-fix is exactly the discipline lesson from v1.1.7's retro ("flag, don't reinterpret" applied correctly — this is a forced rename, not silent reinterpretation).
|
||
|
||
### D2. Trait move skipped (was plan §5)
|
||
|
||
The brief speculated about moving `ExecutorClient` + `ScriptIdentity` from `orchestrator-core` to `shared` to enable an in-engine invoke bridge. Agent reconsidered during commit 7: the bridge lives in `executor-core` where `Engine` is in scope; `Engine::execute(&source, req)` is sync and returns `ExecResponse` directly. Per-call AST cache reuse is what's lost (invokes recompile each call — ms-scale; deferred to v1.2).
|
||
|
||
**Verdict: accept.** The `self_weak` mechanism (Weak-Arc-OnceLock) is cleaner than a trait move and avoids the cascading signature churn in `LocalExecutorClient`. AST recompile cost is a known tradeoff. The brief was speculation; the agent built the simpler shape that works.
|
||
|
||
### D3. Retry columns on parent triggers row only (was plan §1)
|
||
|
||
The brief sketch said `queue_trigger_details` "gets the same five retry override columns as the parent". The parent already has them. Duplicating would split the source of truth.
|
||
|
||
**Verdict: accept; the brief was wrong here.** Single source of truth on the parent is correct. The agent surfaced this per discipline reminder #2.
|
||
|
||
### D4. `Limits::trigger_depth_max` (was plan §5)
|
||
|
||
Added `Limits::trigger_depth_max: u32` (default 8) and synced from `TriggerConfig::from_env().max_trigger_depth` in the picloud binary. `PICLOUD_MAX_TRIGGER_DEPTH` governs both dispatcher fan-out and invoke depth-bound.
|
||
|
||
**Verdict: accept.** Right level of abstraction — depth check inside `Engine` doesn't reach into `TriggerConfig`.
|
||
|
||
### D5. One-consumer-per-queue via advisory lock
|
||
|
||
Partial unique index across `triggers.app_id` and `queue_trigger_details.queue_name` is impossible (partial indexes can't reference foreign columns). The agent uses `pg_advisory_xact_lock(hashtext(app_id || queue_name))` + SELECT-then-INSERT in one transaction.
|
||
|
||
**Verdict: accept.** The xact-scoped advisory lock is auto-released on commit/rollback. Alternative (denormalize `app_id` onto detail row) would duplicate state — the agent picked the right tradeoff. Verified by `queue_e2e::queue_one_consumer_per_queue_rejected`.
|
||
|
||
### D6. `invoke()` exposed globally (not `invoke::*`)
|
||
|
||
The brief itself spelled `invoke(target, args)` not `invoke::call(target, args)`. Registered via `engine.register_global_module(...)`.
|
||
|
||
**Verdict: accept; the brief was self-consistent here.** Scripts write `invoke("worker", #{...})` which matches the brief example.
|
||
|
||
### D7. `invoke_async` runs once (synthetic `retry_max_attempts = 1`)
|
||
|
||
Per the brief: "**NO — `invoke_async` runs once.** Callers who want retries wrap in `retry::with` instead." Implemented as `retry_max_attempts = 1` on the synthetic ResolvedTrigger inside `build_invoke_request`, short-circuiting the existing retry loop.
|
||
|
||
**Verdict: accept.** Surfaced misfires through the dead-letter path; callers retain explicit retry control via `retry::run`.
|
||
|
||
## 4. Substantive strengths
|
||
|
||
**1. The `Engine::self_weak` re-entrancy mechanism is exemplary.** `Weak<Engine>` in a `OnceLock<...>`, set by the picloud binary right after `Arc::new(Engine::new(...))`, upgraded on demand by `self_arc()`. The bridge gets `None` only on engine drop (shutdown). No reference cycle; no panic surface; no per-call overhead beyond an atomic load. Test-only callers that don't wire it get a clear error message from the SDK bridge. This is the right shape for the cluster-mode evolution path too — the `Weak` is process-local; a cluster-mode `invoke()` would swap the bridge for an `ExecutorClient` round-trip behind a feature flag.
|
||
|
||
**2. Cross-app `invoke()` isolation has three layers.** Repo (`get_by_name` takes explicit `app_id`), service (`resolve_id` checks `script.app_id != cx.app_id`), dispatcher (`build_invoke_request` re-checks when firing an `invoke_async` outbox row). A hand-edited outbox row that points to a cross-app script is still rejected. The v1.1.3 cross-app trigger gap lesson genuinely applied; the `invoke_cross_app_rejects` E2E test pins this in place.
|
||
|
||
**3. Queue claim + ack + nack are correctly lease-token-keyed.** Both `ack` and `nack` filter `WHERE id = $1 AND claim_token = $2`. A stale dispatcher whose visibility-timeout expired and lost its lease cannot accidentally delete or re-defer a re-claimed message. The reclaim task's UPDATE...FROM JOIN against the live `(triggers, queue_trigger_details)` set ensures only enabled queue consumers' messages are reclaimed. Race-free in the strong sense.
|
||
|
||
**4. The dispatcher gate-saturation handling for the queue arm.** When the gate is full, the queue arm immediately nacks-with-100ms-delay, keeping the lease accounting consistent and letting the next tick re-claim cleanly. This mirrors the outbox arm's reschedule semantics exactly — symmetric load-shed across both event-firing paths.
|
||
|
||
**5. `retry::policy` clamping is saturating + transparent.** `max_attempts ∈ [1, 20]`, `base_ms ∈ [1, 60_000]`, `jitter_pct ∈ [0, 100]`. Bogus backoff strings surface a clear error pointing at the three valid values. Deterministic jitter via `DefaultHasher` over `(span, raw)` avoids pulling `rand` into the hot path — correctness-equivalent for retry purposes.
|
||
|
||
**6. The 20-commit split is exemplary, again.** Migration → types → repo → service → SDK → dispatcher arm → next service (invoke) → next dispatcher arm → next SDK (retry) → admin HTTP → dashboard → tests → F4 dedup → version bumps → schema bless → test fixes → clippy clean → docs. Each commit independently green. Best commit hygiene of any v1.1.x release alongside v1.1.7 + v1.1.8.
|
||
|
||
**7. Integration test density target met in full (F1).** Four new DB-gated binaries (15 tests), exactly the names the brief specified as non-negotiable. The `queue_visibility_timeout_reclaim` test actually exercises the reclaim by mutating `claimed_at` to simulate a crashed consumer — that's the load-bearing semantic, not a smoke test.
|
||
|
||
## 5. Open questions answered
|
||
|
||
HANDBACK §9 raises four:
|
||
|
||
1. **`retry::run` rename acceptable?** Yes. `run` is clear and unambiguous in this context. `attempt` would have been a contender but reads worse with closures. No change needed.
|
||
|
||
2. **`invoke()` path-resolution method.** Defaulted to POST→GET fallback. Acceptable for v1.1.9; method-aware resolution is a v1.2 ergonomics item (per HANDBACK §11 deferred list).
|
||
|
||
3. **`ctx.trigger_depth` not in `ctx` map.** The depth value IS threaded through `SdkCallCx` correctly (verified by `invoke_depth_limit_exceeds_cleanly`). Surfacing it into Rhai's `ctx` map is a 5-line addition for v1.2.
|
||
|
||
4. **Cluster-mode reclaim coordination.** Out of scope per the brief; v1.3+ cluster work will need advisory-lock coordination for the reclaim task.
|
||
|
||
## 6. Smaller observations
|
||
|
||
- **L1 — pre-existing clippy lints surfaced during F3.** Six lints (two on new v1.1.9 code, four on new v1.1.9 SDK paths) — all fixed in `c3baa87 chore(v1.1.9): clippy clean`. Honest call; the lints were latent until the agent ran clippy properly per F3.
|
||
- **L2 — dashboard pre-existing Playwright errors (149)** in `tests/e2e/**/*.spec.ts`. Same as v1.1.8; not v1.1.9's concern.
|
||
- **`InvokeServiceImpl` constructor signature is 3 args** — well below the 10-arg threshold that triggers builder-pattern temptation. Clean.
|
||
- **Closures-across-`invoke()` rejected at the bridge** with a clear error per HANDBACK §12. Right call — captured environments don't translate across `SdkCallCx` boundaries.
|
||
|
||
## 7. Versioning audit
|
||
|
||
| File | Before | After | Status |
|
||
|---|---|---|---|
|
||
| Workspace `Cargo.toml` | 1.1.8 | 1.1.9 | ✅ |
|
||
| SDK schema (`shared/src/version.rs`) | 1.9 | 1.10 | ✅ Public surface added: `QueueService`, `queue::*`, `invoke()`, `invoke_async()`, `retry::*`, `ctx.event.queue`, `TriggerEvent::Queue` |
|
||
| Dashboard `package.json` | 0.14.0 | 0.15.0 | ✅ |
|
||
| `@picloud/client` | 1.0.0 | 1.0.0 | ✅ (no client work this release, per brief) |
|
||
| Migrations | 0001..0033 | 0034..0035 added | ✅ Sequential |
|
||
| CHANGELOG.md | v1.1.8 entry | v1.1.9 entry (no upgrade constraint — pure additive) | ✅ |
|
||
|
||
## 8. Recommended next steps (post-merge)
|
||
|
||
1. **Merge** `feat/v1.1.9-queues-invoke` into `main` (fast-forward; branch is linear ahead).
|
||
2. **`docker compose down` when convenient** to tear down the dev Postgres container.
|
||
3. **End of v1.1.x.** v1.2 (Workflows & Hierarchies — DAG execution, advanced docs query, interceptors, read triggers, audit log, script-mediated realtime auth, `dead_letters::list`, client-lib type codegen) is the next phase milestone.
|
||
4. **For v1.2 design intake**, fold in:
|
||
- **`ctx.trigger_depth` surface in the Rhai `ctx` map** (5-line addition per HANDBACK §9 #3).
|
||
- **`invoke()` method-aware route resolution** (POST→GET fallback today; explicit method arg per HANDBACK §9 #2).
|
||
- **AST cache reuse across `invoke()` calls** (D2 deferred optimization).
|
||
- **Permission matrix design for `users::*` roles** — v1.1.8 follow-up that's been waiting for the v1.2 design conversation.
|
||
- **Queue mutating admin endpoints** (purge, requeue, delete-message) — needs a payload-viewer UX story.
|
||
5. **For v2 design intake, archive `docs/v1.1.x-design-notes.md`** — every section's decisions have shipped (the doc's own lifecycle note says "Document deleted when v1.1.9 ships").
|
||
|
||
Branch is ready for merge. Verdict: **APPROVE**.
|