6c-2: QA nits — dead code, N+1 query, dep hygiene, k8s hardening #135

New issue

Closed

opened 2026-03-09 13:26:17 +00:00 by forgejo_admin · 1 comment

forgejo_admin commented

2026-03-09 13:26:17 +00:00

Owner

Lineage

plan-2026-02-26-tf-modularize-postgres → Phase 6 (Vector Search) → Phase 6c-2

Repo

forgejo_admin/pal-e-docs

User Story

As a platform operator
I want the embedding worker cleaned up to production quality
So that backfill performance is acceptable and the deployment is properly hardened

Context

QA approved PR #130 (6c: async embedding pipeline) with 7 nits. 2 are no-ops. 5 are actionable. This issue addresses those 5. Depends on PR #130 being merged first.

File Targets

Files to modify:

src/pal_e_docs/embedding_worker.py — nits 1, 2, 3
pyproject.toml — nit 4
k8s/embedding-worker.yaml — nit 6

Acceptance Criteria

Nit 1: EMBEDDABLE_TYPES constant is either used in block filtering logic or removed. Prefer using it.
Nit 2: Redundant set_isolation_level call removed from reconnection path (~line 523). _connect() already handles this.
Nit 3: N+1 note title query replaced with a batch lookup (JOIN or upfront query). Verify with backfill of 5K blocks — should not issue 5K separate title queries.
Nit 4: httpx removed from [project.optional-dependencies] dev section. It's already in main deps.
Nit 6: terminationGracePeriodSeconds: 60 added to k8s Deployment spec. Gives worker time to finish batch and reset processing → pending on shutdown.

Test Expectations

Existing tests still pass: pytest tests/ -k test_embedding
If EMBEDDABLE_TYPES is used: add test verifying only embeddable types are processed
Run command: pytest tests/ -k test_embedding

Constraints

Do NOT change the worker's core logic — this is a cleanup pass, not a feature change
The N+1 fix should use a single query with JOIN, not an in-memory cache that could go stale
Keep terminationGracePeriodSeconds reasonable (60s) — the worker processes batches of 10, each embedding takes <1s

Checklist

PR opened
Tests pass
No unrelated changes
PR description includes Closes #N

phase-postgres-6c2-qa-nits — phase note
PR #130 — the PR these nits came from

### Lineage `plan-2026-02-26-tf-modularize-postgres` → Phase 6 (Vector Search) → Phase 6c-2 ### Repo `forgejo_admin/pal-e-docs` ### User Story As a platform operator I want the embedding worker cleaned up to production quality So that backfill performance is acceptable and the deployment is properly hardened ### Context QA approved PR #130 (6c: async embedding pipeline) with 7 nits. 2 are no-ops. 5 are actionable. This issue addresses those 5. Depends on PR #130 being merged first. ### File Targets Files to modify: - `src/pal_e_docs/embedding_worker.py` — nits 1, 2, 3 - `pyproject.toml` — nit 4 - `k8s/embedding-worker.yaml` — nit 6 ### Acceptance Criteria - [ ] Nit 1: `EMBEDDABLE_TYPES` constant is either used in block filtering logic or removed. Prefer using it. - [ ] Nit 2: Redundant `set_isolation_level` call removed from reconnection path (~line 523). `_connect()` already handles this. - [ ] Nit 3: N+1 note title query replaced with a batch lookup (JOIN or upfront query). Verify with backfill of 5K blocks — should not issue 5K separate title queries. - [ ] Nit 4: `httpx` removed from `[project.optional-dependencies] dev` section. It's already in main deps. - [ ] Nit 6: `terminationGracePeriodSeconds: 60` added to k8s Deployment spec. Gives worker time to finish batch and reset `processing` → `pending` on shutdown. ### Test Expectations - [ ] Existing tests still pass: `pytest tests/ -k test_embedding` - [ ] If EMBEDDABLE_TYPES is used: add test verifying only embeddable types are processed - Run command: `pytest tests/ -k test_embedding` ### Constraints - Do NOT change the worker's core logic — this is a cleanup pass, not a feature change - The N+1 fix should use a single query with JOIN, not an in-memory cache that could go stale - Keep `terminationGracePeriodSeconds` reasonable (60s) — the worker processes batches of 10, each embedding takes <1s ### Checklist - [ ] PR opened - [ ] Tests pass - [ ] No unrelated changes - [ ] PR description includes `Closes #N` ### Related - `phase-postgres-6c2-qa-nits` — phase note - PR #130 — the PR these nits came from

forgejo_admin referenced this issue from a pull request that will close it,

2026-03-09 13:54:06 +00:00

Fix 5 QA nits: dead code, N+1 query, dep hygiene, k8s hardening #136

forgejo_admin commented

2026-03-09 13:59:17 +00:00

Author

Owner

PR #136 Review

PR: "Fix 5 QA nits: dead code, N+1 query, dep hygiene, k8s hardening"
Branch: 135-qa-nits-dead-code-n1-dep-k8s -> main
Files changed: 4 (+52, -27)

BLOCKERS

None.

NITS

Undeclared formatting changes -- The diff includes three changes not listed in the PR summary: collapsing INSTRUCTION_PREFIX from a parenthesized multi-line string to a single line (lines 47-49), reformatting 2 ** attempt to 2**attempt, and joining a multi-line logger string. All are semantically identical and likely from ruff format, but the PR body should mention "ruff format pass" as a change for traceability. Non-blocking.
Comment on line 51 is now slightly misleading -- The comment says "mermaid is skipped by the Postgres trigger" but the code now uses an allowlist (IN %s) rather than a denylist. The trigger may still skip mermaid, but the worker-side filtering is no longer about skipping mermaid -- it is about including only embeddable types. Consider rewording to something like "only these types are fetched for embedding; mermaid is also excluded by the Postgres trigger." Non-blocking.

CODE REVIEW

Nit 1: EMBEDDABLE_TYPES constant and block filtering -- CORRECT.

The constant is defined as {"paragraph", "list", "heading", "table", "code"} (line 50).
_fetch_pending_blocks converts it to a sorted tuple (embeddable = tuple(sorted(EMBEDDABLE_TYPES))) and passes it via IN %s with psycopg2 tuple parameter binding. This is correct psycopg2 usage -- psycopg2 adapts Python tuples to SQL IN (...) clauses.
The shift from denylist (!= 'mermaid') to allowlist (IN %s) is strictly safer: if new non-embeddable block types are added in the future, they are excluded by default rather than silently included.

Nit 2: Redundant set_isolation_level removal -- CORRECT.

_connect() (lines 220-224) already sets ISOLATION_LEVEL_AUTOCOMMIT. The removed call at the reconnection path (old line 523) was a duplicate. No behavioral change.

Nit 3: N+1 note title query replaced with correlated subquery -- CORRECT.

Old code: for each row in the UPDATE RETURNING result, executed a separate SELECT n.title FROM notes n JOIN blocks b ON b.note_id = n.id WHERE b.id = %s. This is N+1.
New code: RETURNING id, block_type, content, (SELECT n.title FROM notes n WHERE n.id = blocks.note_id). In PostgreSQL, the blocks reference in the RETURNING clause refers to the row being updated. This correlated subquery is resolved per-row within the single UPDATE statement, so the database planner can optimize it (e.g., index scan on notes.id PK). The N+1 round-trips are eliminated.
The row unpacking is updated from 3-tuple to 4-tuple accordingly. Correct.

Nit 4: httpx removed from dev deps -- CORRECT.

httpx>=0.27 is present in main dependencies (pyproject.toml line 25). Having it also in [project.optional-dependencies] dev was redundant. Removal is clean.

Nit 5: terminationGracePeriodSeconds: 60 -- CORRECT.

Placed at spec.template.spec.terminationGracePeriodSeconds (line 20 of the k8s manifest). This is the correct location in a Deployment pod spec. The default is 30s; 60s gives the embedding worker more time to finish in-flight Ollama calls and reset processing blocks on SIGTERM. Aligns with the graceful shutdown logic in run_worker().

Tests -- 3 new tests in TestEmbeddableTypes:

test_embeddable_types_matches_extract_logic: verifies every type in the set produces non-empty text from extract_block_text. Good coverage link between the constant and the extraction logic.
test_mermaid_not_in_embeddable_types: explicit negative case.
test_embeddable_types_is_expected_set: snapshot test guards against accidental drift.
All three are unit tests with no DB/network dependency. Appropriate scope for this change.

SOP COMPLIANCE

Branch named after issue (135-qa-nits-dead-code-n1-dep-k8s)
PR body has: Summary, Changes, Test Plan, Related
Related section references plan slug (plan-2026-02-26-tf-modularize-postgres)
Closes #135 present in Related section
Tests exist (3 new) and reported as passing (23 total)
No secrets, .env files, or credentials committed
No unnecessary file changes -- all 4 files are in scope (minor ruff reformatting is acceptable)
Commit messages are descriptive (PR title matches scope)

VERDICT: APPROVED

## PR #136 Review PR: "Fix 5 QA nits: dead code, N+1 query, dep hygiene, k8s hardening" Branch: `135-qa-nits-dead-code-n1-dep-k8s` -> `main` Files changed: 4 (+52, -27) ### BLOCKERS None. ### NITS 1. **Undeclared formatting changes** -- The diff includes three changes not listed in the PR summary: collapsing `INSTRUCTION_PREFIX` from a parenthesized multi-line string to a single line (lines 47-49), reformatting `2 ** attempt` to `2**attempt`, and joining a multi-line logger string. All are semantically identical and likely from `ruff format`, but the PR body should mention "ruff format pass" as a change for traceability. Non-blocking. 2. **Comment on line 51 is now slightly misleading** -- The comment says "mermaid is skipped by the Postgres trigger" but the code now uses an allowlist (`IN %s`) rather than a denylist. The trigger may still skip mermaid, but the worker-side filtering is no longer about skipping mermaid -- it is about including only embeddable types. Consider rewording to something like "only these types are fetched for embedding; mermaid is also excluded by the Postgres trigger." Non-blocking. ### CODE REVIEW **Nit 1: EMBEDDABLE_TYPES constant and block filtering** -- CORRECT. - The constant is defined as `{"paragraph", "list", "heading", "table", "code"}` (line 50). - `_fetch_pending_blocks` converts it to a sorted tuple (`embeddable = tuple(sorted(EMBEDDABLE_TYPES))`) and passes it via `IN %s` with psycopg2 tuple parameter binding. This is correct psycopg2 usage -- psycopg2 adapts Python tuples to SQL `IN (...)` clauses. - The shift from denylist (`!= 'mermaid'`) to allowlist (`IN %s`) is strictly safer: if new non-embeddable block types are added in the future, they are excluded by default rather than silently included. **Nit 2: Redundant `set_isolation_level` removal** -- CORRECT. - `_connect()` (lines 220-224) already sets `ISOLATION_LEVEL_AUTOCOMMIT`. The removed call at the reconnection path (old line 523) was a duplicate. No behavioral change. **Nit 3: N+1 note title query replaced with correlated subquery** -- CORRECT. - Old code: for each row in the UPDATE RETURNING result, executed a separate `SELECT n.title FROM notes n JOIN blocks b ON b.note_id = n.id WHERE b.id = %s`. This is N+1. - New code: `RETURNING id, block_type, content, (SELECT n.title FROM notes n WHERE n.id = blocks.note_id)`. In PostgreSQL, the `blocks` reference in the RETURNING clause refers to the row being updated. This correlated subquery is resolved per-row within the single UPDATE statement, so the database planner can optimize it (e.g., index scan on `notes.id` PK). The N+1 round-trips are eliminated. - The row unpacking is updated from 3-tuple to 4-tuple accordingly. Correct. **Nit 4: httpx removed from dev deps** -- CORRECT. - `httpx>=0.27` is present in main `dependencies` (pyproject.toml line 25). Having it also in `[project.optional-dependencies] dev` was redundant. Removal is clean. **Nit 5: `terminationGracePeriodSeconds: 60`** -- CORRECT. - Placed at `spec.template.spec.terminationGracePeriodSeconds` (line 20 of the k8s manifest). This is the correct location in a Deployment pod spec. The default is 30s; 60s gives the embedding worker more time to finish in-flight Ollama calls and reset processing blocks on SIGTERM. Aligns with the graceful shutdown logic in `run_worker()`. **Tests** -- 3 new tests in `TestEmbeddableTypes`: - `test_embeddable_types_matches_extract_logic`: verifies every type in the set produces non-empty text from `extract_block_text`. Good coverage link between the constant and the extraction logic. - `test_mermaid_not_in_embeddable_types`: explicit negative case. - `test_embeddable_types_is_expected_set`: snapshot test guards against accidental drift. - All three are unit tests with no DB/network dependency. Appropriate scope for this change. ### SOP COMPLIANCE - [x] Branch named after issue (`135-qa-nits-dead-code-n1-dep-k8s`) - [x] PR body has: Summary, Changes, Test Plan, Related - [x] Related section references plan slug (`plan-2026-02-26-tf-modularize-postgres`) - [x] `Closes #135` present in Related section - [x] Tests exist (3 new) and reported as passing (23 total) - [x] No secrets, .env files, or credentials committed - [x] No unnecessary file changes -- all 4 files are in scope (minor ruff reformatting is acceptable) - [x] Commit messages are descriptive (PR title matches scope) ### VERDICT: APPROVED

forgejo_admin closed this issue

2026-03-09 14:00:28 +00:00