Remove legacy jobs and RunLog observability residue - #1215
Conversation
📝 WalkthroughWalkthroughThe PR makes RunLog authoritative for job execution metrics, removes legacy observability columns from D1, adds RunLog-based job health totals to admin insights, and upgrades the RunLog schema to version 9. ChangesJob observability migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant AdminInsightsData
participant D1
participant RunLog
participant AdminSnapshot
AdminInsightsData->>D1: Read total and enabled job schedules
AdminInsightsData->>RunLog: Read bounded per-user job outcome snapshots
RunLog-->>AdminInsightsData: Return success and error totals
AdminInsightsData->>AdminSnapshot: Return combined job health metrics
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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. Comment |
|
🔎 Preview deployed: https://kody-pr-1215.kody-a99.workers.dev Worker: Mocks:
|
|
@coderabbitai review |
1 similar comment
|
@coderabbitai review |
✅ Action performedReview finished.
|
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
541468c to
e44e883
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/worker/src/run-records/dedicated-state.workers.test.ts (1)
1251-1254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
test.eachfor the warm schema upgrade cases.The loop runs the v7 and v8 paths under one test name. If v8 fails, the failure does not identify the installed version.
Inline
legacy_seededunless a repo guard requires the join.
['legacy', 'seeded'].join('_')hides the column name inside the test, while repo config does not reference a legacy-column guard.♻️ Proposed refactor to parameterize the test
-test('warm schema v7 and v8 objects upgrade to v9 without losing job data', async () => { - const retiredColumn = ['legacy', 'seeded'].join('_') - for (const installedVersion of [7, 8]) { +const retiredColumn = 'legacy_seeded' + +test.each([7, 8])( + 'warm schema v%i object upgrades to v9 without losing job data', + async (installedVersion) => { const userId = uniqueUserId(`schema-v${installedVersion}-warm`)Then close the callback where the loop currently ends and remove one indentation level.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/run-records/dedicated-state.workers.test.ts` around lines 1251 - 1254, Refactor the warm schema upgrade test to use test.each with installed versions 7 and 8, so each case reports its version independently; remove the loop and adjust the callback body indentation while preserving the existing assertions and job-data behavior. Replace the retiredColumn join expression with the literal legacy_seeded, unless an existing repository guard requires retaining the join.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/worker/src/run-records/dedicated-state.workers.test.ts`:
- Around line 1251-1254: Refactor the warm schema upgrade test to use test.each
with installed versions 7 and 8, so each case reports its version independently;
remove the loop and adjust the callback body indentation while preserving the
existing assertions and job-data behavior. Replace the retiredColumn join
expression with the literal legacy_seeded, unless an existing repository guard
requires retaining the join.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 845d4dd7-376d-4199-a383-3c4d1588a322
📒 Files selected for processing (20)
docs/contributing/architecture/run-records.mdpackages/worker/migrations/0142-drop-jobs-observability-columns.sqlpackages/worker/src/app/admin-insights-data.node.test.tspackages/worker/src/app/admin-insights-data.tspackages/worker/src/community/community-flow-test-schema.tspackages/worker/src/entitlements/service.tspackages/worker/src/jobs/job-run-observability-hydrate.node.test.tspackages/worker/src/jobs/job-run-observability-hydrate.tspackages/worker/src/jobs/process-due-jobs.tspackages/worker/src/jobs/repo.tspackages/worker/src/jobs/repo.workers.test.tspackages/worker/src/jobs/run-due-jobs-claim-fence.node.test.tspackages/worker/src/jobs/service.node.test.tspackages/worker/src/mcp/run-kody-registry.node.test.tspackages/worker/src/run-records/admin-insights-snapshot.tspackages/worker/src/run-records/dedicated-state.workers.test.tspackages/worker/src/run-records/run-log-do.tspackages/worker/src/run-records/service.node.test.tspackages/worker/src/run-records/service.tstools/migration-ledger.json
💤 Files with no reviewable changes (2)
- packages/worker/src/jobs/run-due-jobs-claim-fence.node.test.ts
- packages/worker/src/jobs/process-due-jobs.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/worker/src/community/community-flow-test-schema.ts
- packages/worker/src/entitlements/service.ts
- packages/worker/src/run-records/admin-insights-snapshot.ts
- packages/worker/src/run-records/service.node.test.ts
- packages/worker/src/jobs/job-run-observability-hydrate.ts
- tools/migration-ledger.json
- docs/contributing/architecture/run-records.md
- packages/worker/src/jobs/job-run-observability-hydrate.node.test.ts
- packages/worker/src/app/admin-insights-data.ts
- packages/worker/src/mcp/run-kody-registry.node.test.ts
- packages/worker/src/jobs/service.node.test.ts
- packages/worker/src/jobs/repo.workers.test.ts
- packages/worker/src/app/admin-insights-data.node.test.ts
- packages/worker/src/run-records/run-log-do.ts
- packages/worker/src/jobs/repo.ts
- packages/worker/src/run-records/service.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/worker/migrations/0144-drop-jobs-observability-columns.sql`:
- Around line 4-8: Reconcile the DROP COLUMN statements in migration 0144 with
the actual pre-0144 jobs schema, removing or correcting references to columns
such as last_run_error that do not exist so the migration completes cleanly.
After updating the migration, regenerate the corresponding checksum entry in
tools/migration-ledger.json.
- Around line 4-8: Update the migration before the DROP COLUMN statements to
backfill existing jobs into job_run_observability, deriving the required userId
and jobId values and migrating the aggregate observability fields from
last_run_error, last_duration_ms, run_count, success_count, and error_count.
Ensure the backfill completes before removing those jobs columns.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b04455b8-c298-468f-818a-5d18dfe3dae6
📒 Files selected for processing (2)
packages/worker/migrations/0144-drop-jobs-observability-columns.sqltools/migration-ledger.json
| ALTER TABLE jobs DROP COLUMN last_run_error; | ||
| ALTER TABLE jobs DROP COLUMN last_duration_ms; | ||
| ALTER TABLE jobs DROP COLUMN run_count; | ||
| ALTER TABLE jobs DROP COLUMN success_count; | ||
| ALTER TABLE jobs DROP COLUMN error_count; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Fix the jobs column contract before applying migration 0144.
Preview deployment fails at Line 4 with SQLite error - no such column: "last_run_error". The migration stops before it applies the remaining drops. Reconcile these statements with the schema produced before migration 0144, then update the checksum in tools/migration-ledger.json at Lines 584-585. Do not deploy until a clean preview applies the migration.
🧰 Tools
🪛 GitHub Actions: 🔎 Preview / 🔎 Deploy Preview Resources
[error] Database migration failed: A request to the Cloudflare API failed. SQLite error - no such column: "last_run_error" at offset 32 [code: 7500]. The migration attempted to reference a column that does not exist in the database schema. Verify that the database schema matches the expected state before applying this migration.
🪛 Squawk (2.61.0)
[warning] 4-4: Dropping a column may break existing clients.
(ban-drop-column)
[warning] 5-5: Dropping a column may break existing clients.
(ban-drop-column)
[warning] 6-6: Dropping a column may break existing clients.
(ban-drop-column)
[warning] 7-7: Dropping a column may break existing clients.
(ban-drop-column)
[warning] 8-8: Dropping a column may break existing clients.
(ban-drop-column)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/worker/migrations/0144-drop-jobs-observability-columns.sql` around
lines 4 - 8, Reconcile the DROP COLUMN statements in migration 0144 with the
actual pre-0144 jobs schema, removing or correcting references to columns such
as last_run_error that do not exist so the migration completes cleanly. After
updating the migration, regenerate the corresponding checksum entry in
tools/migration-ledger.json.
Source: Pipeline failures
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'0144|last_run_error|last_duration_ms|run_count|success_count|error_count|last_error_at|total_duration_ms|JobRunObservability|job_run_observability|legacySeeded' \
packages/worker/migrations packages/worker/src tools/migration-ledger.jsonRepository: kentcdodds/kody
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## migration files around 0144"
ls -1 packages/worker/migrations/*.sql | sort | grep -E '(0142|0143|0144|0145)' -n || true
echo
echo "## 0144 + 3 adjacent migrations"
for f in packages/worker/migrations/0142*.sql packages/worker/migrations/0143*.sql packages/worker/migrations/0144*.sql packages/worker/migrations/0145*.sql; do
[ -f "$f" ] || continue
echo "### $f"
sed -n '1,220p' "$f"
done
echo
echo "## job_run_observability migrations in packages/worker/migrations"
rg -n -C 4 'job_run_observability|last_run_error|last_duration_ms|run_count|success_count|error_count' packages/worker/migrations || true
echo
echo "## migration-ledger entries near observability schema"
python3 - <<'PY'
import json
from pathlib import Path
p = Path('tools/migration-ledger.json')
if p.exists():
data=json.loads(p.read_text())
print(data[:28])
else:
print('missing tools/migration-ledger.json')
PY
echo
echo "## source references for legacy D1 fields / backfill"
rg -n -C 3 'legacySeeded|JobRunObservabilityRecord|JobRunObservabilityUpsertInput|upsertJobRunObservability\(|getJobRunObservability\(|last_run_error|last_duration_ms|run_count|success_count|error_count' packages/worker/src packages/worker/migrations | head -n 240Repository: kentcdodds/kody
Length of output: 21017
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## migrations before 150"
ls -1 packages/worker/migrations/*.sql | sort | awk 'NR>=120 && NR<=150' | while read f; do
echo "### $f"
sed -n '1,180p' "$f"
done
echo
echo "## all migration-ledger entries"
python3 - <<'PY'
import json
from pathlib import Path
p = Path('tools/migration-ledger.json')
if not p.exists():
print('missing tools/migration-ledger.json')
raise SystemExit(0)
data = json.loads(p.read_text())
by_id = {}
for item in data:
mid = item.get('id')
if mid:
by_id[vid] = item
continue
by_id[f"#{len(data)}"] = item
for i, item in enumerate(data, 1):
if "observability" in str(item).lower() or "run_log" in str(item).lower() or "job_run" in str(item).lower() or "run_count" in str(item).lower() or "success_count" in str(item).lower() or "error_count" in str(item).lower() or "last_duration_ms" in str(item).lower() or "last_run_error" in str(item).lower():
print(f"#{i}: {item}")
print(f"\nTotal ledger entries: {len(data)}")
PY
echo
echo "## deterministic scan for D1->run_log job observability backfill"
python3 - <<'PY'
from pathlib import Path
import re
fields = {
"last_run_error": [],
"last_duration_ms": [],
"run_count": [],
"success_count": [],
"error_count": [],
}
tables = {"jobs", "job_run_observability", "job_run_observability_v9"}
for path in sorted(Path("packages/worker/migrations").glob("*.sql")):
text = path.read_text()
for field, rows in fields.items():
if field in text:
rows.append(path.name)
if "job_run_observability" in text:
print(f"job_observability_refs: {path.name}")
for m in re.finditer(r"(CREATE TABLE|DROP TABLE|ALTER TABLE|INSERT INTO),", text):
print(" ", text[m.start()-60:m.start()], text[m.start():m.start()+120])
print("\ndiffs_migrating_fields_from_jobs:")
for field, paths in fields.items():
backfills = [p for p in paths if p.startswith(("0021", "0026")) or "backfill" in p.read_text().lower() or "backup" in p.read_text().lower()]
print(field, ":", paths)
print("\nrun_log_schema_tables:")
for path in sorted(Path("packages/worker/migrations").glob("*.sql")):
text = path.read_text()
for table in tables:
if f"CREATE TABLE `{table}`" in text or f"CREATE TABLE IF NOT EXISTS `{table}`" in text or table in text:
print(f"{path.name}: {table}")
print(text[:800])
print("\nrun_log_v9_refs:")
for path in sorted(Path("packages/worker/migrations").glob("*.sql")):
text = path.read_text()
if "v9" in text.lower() or "rebuildJobRunObservabilityForV9" in text:
print(path.name)
PY
echo
echo "## source backfill/search concise"
rg -n 'backfill|backup|copy|populate|upsertJobRunObservability|getJobRunObservability|JobRunObservabilityUpsertInput|legacySeeded|run_count|success_count|error_count|last_duration_ms|last_run_error' packages/worker/src packages/worker/scripts || trueRepository: kentcdodds/kody
Length of output: 50371
Add a D1-to-run-log observability migration before dropping these columns.
0144-drop-jobs-observability-columns.sql removes last_run_error, last_duration_ms, run_count, success_count, and error_count from jobs without populating job_run_observability. Existing job records will lose historical D1 observability after this drop, so backfill userId/jobId and aggregate values into job_run_observability first.
🧰 Tools
🪛 GitHub Actions: 🔎 Preview / 🔎 Deploy Preview Resources
[error] Database migration failed: A request to the Cloudflare API failed. SQLite error - no such column: "last_run_error" at offset 32 [code: 7500]. The migration attempted to reference a column that does not exist in the database schema. Verify that the database schema matches the expected state before applying this migration.
🪛 Squawk (2.61.0)
[warning] 4-4: Dropping a column may break existing clients.
(ban-drop-column)
[warning] 5-5: Dropping a column may break existing clients.
(ban-drop-column)
[warning] 6-6: Dropping a column may break existing clients.
(ban-drop-column)
[warning] 7-7: Dropping a column may break existing clients.
(ban-drop-column)
[warning] 8-8: Dropping a column may break existing clients.
(ban-drop-column)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/worker/migrations/0144-drop-jobs-observability-columns.sql` around
lines 4 - 8, Update the migration before the DROP COLUMN statements to backfill
existing jobs into job_run_observability, deriving the required userId and jobId
values and migrating the aggregate observability fields from last_run_error,
last_duration_ms, run_count, success_count, and error_count. Ensure the backfill
completes before removing those jobs columns.
There was a problem hiding this comment.
Resolved operationally and verified on a clean preview. This PR’s persisted preview D1 had already applied the identical migration under its pre-rebase 0142 filename, so the renamed 0144 replay correctly found the columns absent. Production never applied 0142. I cleaned only PR #1215’s disposable preview resources and reran deployment from an empty preview database; migration 0144 and the full preview deploy now pass: https://github.com/kentcdodds/kody/actions/runs/30925518579/job/92049383782. Local npm run validate also passes from fresh E2E D1 state.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
3 participants
Intent
Complete the jobs/RunLog legacy-observability hard cut from #1069 so RunLog is the only source for job error, duration, and counters at launch.
Summary
0144to drop the five quiescent jobs observability columns while retaininglast_run_atandlast_run_status(0142was renumbered after0143landed first)job_run_observabilityat RunLog schema v9 withoutlegacy_seededTesting
npm run validate— passed on the final rebased revision with fresh E2E D1 state (553 files / 1,855 tests, 2 MCP files / 3 tests, 7 E2E tests, plus format, lint, typecheck, backup build, primitives, migrations, and docs)npx vitest run packages/worker/src/entitlements/d1-storage-reconciliation.workers.test.ts— 6 passednpx vitest run packages/worker/src/community/community-flow.workers.test.ts— 2 passed01440144reported ✅ and healthcheck served merge SHA984b2fccSystem changes
System recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@e090800e· Head:7e2c2353Classification: extends — removes retired D1 storage and changes the RunLog Durable Object schema while preserving the public jobs observability contract.
Primitives touched
jobsrun-recordsd1-app-db0144drops five quiescent jobs columnsentitlementsapp-uimcp-serverSystem map
Job inspection overlays RunLog observability on D1 schedule anchors; admin insights folds content-free RunLog counters; migration
0144removes retired D1 copies.Legend: green = composes (wiring only) · amber = extended by this PR · red = new primitive · gray = context (unchanged, included only when an edge crosses it).
Before / after
last_run_at/last_run_statusanchorslegacy_seededInvariants
state-vs-history: scheduling and retention anchors remain on the job entity; observability remains dedicated RunLog state outside pruned run history.per-user-isolation: admin aggregation continues bounded per-user RunLog point reads without names, errors, or logs.Conductor report
STATUS done — squash-merged as
984b2fcc; final local validation, CI, AI review, clean preview, main validation, and production deploy are green. Production deploy applied0144-drop-jobs-observability-columns.sqlsuccessfully and healthchecked the merge SHA.legacy_seededis absent outside immutable history, D1 job observability readers/writers are removed, and RunLog is the sole error/duration/counter source. Tracks #1069.Summary by CodeRabbit
Refactor
Tests