Skip to content

fix(office): stop the stuck-parent wake sweep from failing every tick on Postgres - #3525

Open
nova28 wants to merge 3 commits into
kdlbs:mainfrom
nova28:feature/office-pg-liststuckparents-6a49a8
Open

fix(office): stop the stuck-parent wake sweep from failing every tick on Postgres#3525
nova28 wants to merge 3 commits into
kdlbs:mainfrom
nova28:feature/office-pg-liststuckparents-6a49a8

Conversation

@nova28

@nova28 nova28 commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Tip

PR walkthrough: Open the visual walkthrough

Today: On PostgreSQL, the parent-wake reconciler's ListStuckParents query fails at parse time (syntax error at or near "'b'"), so ParentWakeReconciler — the level-triggered backstop that recovers stuck parent tasks — logs an error on every tick and never recovers anything. SQLite is unaffected, which is why CI stayed green.
After this: The query parses and runs correctly on both engines; the reconciler backstop works on Postgres exactly as it already does on SQLite.
Who hits this: Anyone running Office on Postgres — every tick of the reconciler, silently, until a parent gets permanently stuck with no automatic recovery.
Scope: Standalone. Two of four related dialect issues in this query already have owners on other PRs (IS NOTIS DISTINCT FROM landed via #3290; the wsp.rowid fallback fixed via #3459). This PR is the remaining two: GROUP_CONCAT and json_extract.
Not here: A sqlguard rule to catch this class of dialect leak (GROUP_CONCAT / json_extract / IS NOT <column>) at CI time before it merges — filed as a follow-up (task 01218dab), not this PR, to keep this change a pure bugfix.

The parent-wake backstop query used two SQLite-only SQL constructs Postgres rejects outright:

  • GROUP_CONCAT(...) (no Postgres equivalent) → new childSetKeyAggregate(driver) picks STRING_AGG(c.id || ':' || c.state, ',' ORDER BY c.id) on Postgres, and the byte-identical unchanged GROUP_CONCAT form on SQLite.
  • json_extract(w.payload, '$.task_id')dialect.JSONExtract(driver, "w.payload", "task_id"), the existing helper already used for this exact pattern elsewhere in the repo (failure.go, runs_inflight.go, participants.go).

A new Postgres-gated test file (wake_receipts_postgres_test.go, KANDEV_TEST_POSTGRES_DSN-gated per apps/backend/AGENTS.md) exercises ListStuckParents and the STRING_AGG aggregate against a real PostgreSQL instance. It runs in CI's postgres-boot job (PostgreSQL 16) — internal/office/repository/sqlite is in that job's explicit POSTGRES_PACKAGES list.

Validation

  • go build ./... — clean
  • go vet ./internal/office/repository/sqlite/... — clean
  • go test -tags fts5 ./internal/office/repository/sqlite/ -run 'StuckParent|WakeReceipt|ChildSetKey' -count=1 -v — 9 SQLite tests pass; 3 Postgres tests skip locally (no KANDEV_TEST_POSTGRES_DSN), run against real Postgres 16 in CI's postgres-boot job
  • go run ./cmd/sqlguard ./internal — clean (this class of dialect leak isn't yet covered — see follow-up above)
  • gofmt -l on both changed files — clean
  • make typecheck test lint — clean, other than pre-existing failures proven identical against origin/main in a scratch worktree (macOS-runner-specific internal/worktree/agentctl/launcher/etc. TMPDIR path-safety failures, and TestMigrate_PriorityIdempotent under -tags fts5) — none touch the changed files or packages
  • make lint-format — clean
  • pnpm run i18n:ratchet — no UI source added or modified
  • E2E not required: diff is two Go files under apps/backend/internal/office/repository/sqlite/, zero apps/web/ paths

Possible Improvements

Low risk. One residual, non-blocking: the new STRING_AGG regression test pins ordering via the derived table's own ORDER BY id as much as via STRING_AGG(... ORDER BY c.id) itself, so it's a slightly weaker regression pin than it looks (analysis-level observation, not verified against a live Postgres in this environment).

Checklist

  • If I do not have repository write access and this is a large architectural change, I discussed the direction in a linked issue before opening this PR.
  • This PR contains one logical change; unrelated work is split into separate PRs.
  • I have performed a self-review of my code.
  • I have manually tested my changes and they work as expected.
  • My changes have tests that cover the new functionality and edge cases.
  • If my change touches UI files (apps/web/), I have added or updated Playwright e2e tests in apps/web/e2e/ and verified them with make test-e2e.
  • I checked whether this affects public docs in docs/public/** and updated them or noted why no docs change is needed.

Review in cubic

Preview Environment

URL https://kandev-pr-3525-bwo7.sprites.app
Commit a4af353
Agent Mock agent

Updates automatically on each push. Destroyed when the PR is closed.

nova28 and others added 2 commits September 9, 2026 08:07
ListStuckParents backs ParentWakeReconciler, the level-triggered backstop
that re-delivers a task_children_completed wake to a parent whose children
have all finished. On PostgreSQL the query could not run at all: it carried
three SQLite-only constructs, each rejected at parse time, so every tick
logged an error and no parent was ever recovered.

- GROUP_CONCAT has no PostgreSQL equivalent; render STRING_AGG there. The
  ordering is restated inside the aggregate because PostgreSQL does not
  inherit it from the subquery, and the result has to match the key
  formatChildSetKey builds on the write side byte for byte.
- IS NOT with a column right-hand side is a SQLite extension; IS DISTINCT
  FROM is standard and means the same thing on both engines.
- json_extract is SQLite-only; use the existing dialect.JSONExtract helper,
  the same remedy already applied to HasPriorTasklessFailedRun.

Adds PostgreSQL behaviour tests covering the sweep, the receipt and
in-flight-run exclusions, and the equality between the SQL-side and Go-side
child-set keys. They skip unless KANDEV_TEST_POSTGRES_DSN is set.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ipt tests

newPostgresWakeRepo initialised only the task and office repositories, so
agent_profiles (owned by the settings store) and
workflow_step_participants.created_at (added by the workflow repository's
migration, and read by RunnerProjection's third COALESCE arm) never existed.
seedPostgresRunner also inserted agent_profiles with an empty agent_id,
violating the FK to agents(id) that Postgres enforces. All three tests in
wake_receipts_postgres_test.go failed for these harness reasons, not because
of the production query.
@nova28
nova28 temporarily deployed to opencode-review-trusted September 9, 2026 00:10 — with GitHub Actions Inactive
@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: QUIET

Plan: Advanced

Run ID: 5620f3fe-df7d-47bd-b0ff-db23e78a3cc4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: QUIET

Plan: Advanced

Run ID: f3d95b46-e5a2-4b38-a828-9cf537a49d76

📥 Commits

Reviewing files that changed from the base of the PR and between 401947f and 1347636.

📒 Files selected for processing (2)
  • apps/backend/internal/office/repository/sqlite/wake_receipts.go
  • apps/backend/internal/office/repository/sqlite/wake_receipts_postgres_test.go

Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review.


📝 Summary

Summary by CodeRabbit

  • Bug Fixes

    • Improved detection of stuck parent receipts when using PostgreSQL.
    • Ensured child task ordering and receipt comparisons produce consistent results across supported databases.
    • Corrected task ID extraction from run payloads on PostgreSQL.
  • Tests

    • Added PostgreSQL coverage for stuck receipt detection, child ordering, and covered-parent exclusion.

Walkthrough

ListStuckParents now generates PostgreSQL-compatible SQL while preserving SQLite support. New PostgreSQL integration tests cover query execution, child-set ordering, and exclusion of covered parents.

Changes

Wake receipt query compatibility

Layer / File(s) Summary
Dialect-aware stuck-parent query
apps/backend/internal/office/repository/sqlite/wake_receipts.go
ListStuckParents selects driver-specific child-set aggregation and JSON extraction. Receipt comparison uses IS DISTINCT FROM.
PostgreSQL integration validation
apps/backend/internal/office/repository/sqlite/wake_receipts_postgres_test.go
Tests initialize PostgreSQL repositories, seed required records, verify child-set ordering, and check covered-parent exclusions.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 13476

Stuck parent tasks can now be identified and recovered on PostgreSQL while preserving SQLite behavior. The query’s aggregation, receipt handling, and wake-run exclusions are covered, with no current merge-blocking risk identified.

Sequence Diagram(s)

sequenceDiagram
  participant ListStuckParents
  participant childSetKeyAggregate
  participant PostgreSQL
  ListStuckParents->>childSetKeyAggregate: select ordered child-set aggregate
  childSetKeyAggregate->>PostgreSQL: render STRING_AGG SQL
  ListStuckParents->>PostgreSQL: execute dialect-aware query
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the PostgreSQL bug and the affected stuck-parent wake sweep.
Description check ✅ Passed The description explains the problem, scope, implementation, validation, risks, and checklist. It is detailed and aligned with the pull request objectives.
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 2 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

A rabbit sorts child IDs in line
PostgreSQL joins them fine
Receipts compare, nulls behave
Wake runs leave covered parents saved
Tests hop through schemas bright

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Claude finished @nova28's task in 3m 14s —— View job


Code Review

  • Gather context and read changed files
  • Review wake_receipts.go changes
  • Review wake_receipts_postgres_test.go
  • Check for correctness, dialect safety, and test coverage
  • Post findings

Review Summary

LGTM. The fix is correct, minimal, and follows established patterns in the codebase. Two findings below — one is a minor doc inaccuracy, one an acknowledged limitation from the PR description.

wake_receipts.go

The two dialect fixes are correct:

GROUP_CONCATchildSetKeyAggregate: The new helper correctly branches on dialect.IsPostgres. On Postgres, STRING_AGG(c.id || ':' || c.state, ',' ORDER BY c.id) is the right form — and the comment correctly explains why the ORDER BY is restated inside the aggregate rather than relying on the derived table's own ORDER BY id. SQLite preserves subquery ordering for aggregates; Postgres does not. The byte-for-byte invariant against formatChildSetKey (the Go path that writes receipts) is clearly documented.

json_extractdialect.JSONExtract: Uses the existing helper already used in failure.go, runs_inflight.go, and participants.go. Correct.

IS DISTINCT FROM (line 146) was already present from #3290 — not touched here, as expected.

wake_receipts_postgres_test.go

Setup order (newPostgresWakeRepo): settings → task → workflow → office is correct for Postgres FK enforcement. The comment explains why workflow_step_participants.created_at needs the workflow repo initialized — matches the pattern in participant_claim_postgres_test.go.

seedPostgresRunner: Correctly inserts an agents row before agent_profiles because Postgres enforces the FK that SQLite's test harness does not. Good call-out in the comment.

Three test cases cover:

  • Basic detection of a stuck parent (smoke test)
  • Key ordering invariant — children inserted out-of-id-order to actually exercise the ORDER BY c.id inside STRING_AGG
  • Receipt-match and queued-run exclusion predicates, including the json_extract/->> arm

Findings

1. Minor: test header comment counts three constructs, but there were four

The comment at TestPostgresListStuckParents line 17–20 says the query "carried three SQLite-only constructs — GROUP_CONCAT, json_extract(...), and IS NOT with a column right-hand side." Per the PR description there were originally four: GROUP_CONCAT, json_extract, IS NOT (→ IS DISTINCT FROM, fixed in #3290), and wsp.rowid (fixed in #3459). The number "three" and the omission of wsp.rowid make the count imprecise. Minor — doesn't affect correctness.

2. Acknowledged: STRING_AGG ordering test is a slightly weaker pin

As noted in the PR description: because the derived table also has ORDER BY id, a Postgres planner that happens to preserve that order would make the test pass even if the ORDER BY c.id inside STRING_AGG were accidentally removed. The test does insert children out-of-order intentionally and the assertion is meaningful, but the pin is somewhat dependent on planner behavior for the negative case. This is the limitation acknowledged in "Possible Improvements" — not a blocking issue.

Both are non-blocking. The core fix is sound and the test file adds real Postgres coverage for constructs that were previously breaking the reconciler on every tick.

@greptile-apps

greptile-apps Bot commented Sep 9, 2026

Copy link
Copy Markdown

Greptile Summary

The PR restores the stuck-parent reconciliation query on PostgreSQL by selecting dialect-compatible aggregate and JSON-extraction expressions, while preserving SQLite behavior.

  • Uses ordered STRING_AGG on PostgreSQL and the existing GROUP_CONCAT expression on SQLite.
  • Uses the shared JSON extraction helper for the in-flight wake-run predicate.
  • Adds real-PostgreSQL regression coverage for candidate selection, aggregate ordering, receipt matching, and queued-run exclusion.
  • The implementation appears behaviorally correct; only the production-comment convention needs adjustment.

Confidence Score: 4/5

The SQL fix appears behaviorally safe, but the explicit production-comment requirement should be satisfied before merging.

The dialect branches match the supported database drivers and the PostgreSQL tests cover the repaired predicates; the only accepted issue is a repository-rule violation in the new production comment.

Files Needing Attention: apps/backend/internal/office/repository/sqlite/wake_receipts.go

Important Files Changed

Filename Overview
apps/backend/internal/office/repository/sqlite/wake_receipts.go Adds correct dialect-specific SQL generation for the parent-wake sweep; its new production comment should be reduced to the invariant.
apps/backend/internal/office/repository/sqlite/wake_receipts_postgres_test.go Adds PostgreSQL-gated integration coverage for query parsing, deterministic child-set keys, receipt matching, and queued-run exclusion.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  Tick[Parent wake reconciliation tick] --> Driver{Repository driver}
  Driver -->|pgx| PG[Ordered STRING_AGG and JSONB extraction]
  Driver -->|sqlite3| SQLite[GROUP_CONCAT and json_extract]
  PG --> Query[ListStuckParents]
  SQLite --> Query
  Query --> Filter[Exclude matching receipts and in-flight wake runs]
  Filter --> Candidates[Return parents requiring recovery]
Loading

Reviews (1): Last reviewed commit: "fix(office): boot settings and workflow ..." | Re-trigger Greptile

Review nit: the comment claimed three SQLite-only constructs but the
PR description names four (wsp.rowid already fixed in kdlbs#3459).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@nova28
nova28 temporarily deployed to opencode-review-trusted September 9, 2026 01:11 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant