PiCloud/REVIEW.md

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, 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 + dispatcher.rs::tick_queue_arm	✅ 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	✅ 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	✅ 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	✅ Same lease-token filter; idempotent
Visibility-timeout reclaim every PICLOUD_QUEUE_RECLAIM_INTERVAL_MS (default 30s)	dispatcher.rs::spawn lines 87-104 + queue_repo.rs::reclaim_visibility_timeouts	✅ 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	✅ 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 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 (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 + 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	✅ Saturating clamps with clear errors on bogus backoff strings
retry::run(policy, fn_ptr) re-invokes FnPtr through NativeCallContext	retry.rs::retry_run	✅ 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 + 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	✅
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/ + 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.