diff --git a/CHANGELOG.md b/CHANGELOG.md index 0252119..0f315a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,9 @@ All notable changes to ShellClaw are documented here. Format follows [Keep a Cha ## [Unreleased] +### Fixed +- Cron jobs whose channel cannot deliver (unknown name, a channel left disabled, or `cron` itself) are rejected at create time (`-2`). A row already stored with that channel is disabled on the next poll before `agent_run`. Previously `cron_send` failed after the full turn, ack was skipped, and the still-due job reran the model and its tools on every poll. + ## [1.0.1] - 2026-10-18 ### Fixed diff --git a/src/core/memory.c b/src/core/memory.c index 8e7f0ab..c8f7fbf 100644 --- a/src/core/memory.c +++ b/src/core/memory.c @@ -462,6 +462,22 @@ int cron_job_toggle(const char *id) return ret; } +int cron_job_set_enabled(const char *id, int enabled) +{ + const char *sql = "UPDATE cron_jobs SET enabled = ?1 WHERE id = ?2 AND enabled != ?1"; + sqlite3_stmt *stmt = NULL; + int ret; + + if (!g_db || !id) return -1; + memory_sqlite_entry_probe(); + if (sqlite3_prepare_v2(g_db, sql, -1, &stmt, NULL) != SQLITE_OK) return -1; + sqlite3_bind_int(stmt, 1, enabled ? 1 : 0); + sqlite3_bind_text(stmt, 2, id, -1, SQLITE_TRANSIENT); + ret = (sqlite3_step(stmt) == SQLITE_DONE && sqlite3_changes(g_db) > 0) ? 0 : -1; + sqlite3_finalize(stmt); + return ret; +} + int cron_job_update_next_run(const char *id, long long next_run) { if (!g_db || !id) return -1; diff --git a/src/core/memory.h b/src/core/memory.h index e433fac..c098088 100644 --- a/src/core/memory.h +++ b/src/core/memory.h @@ -181,6 +181,18 @@ int cron_job_delete(const char *id); */ int cron_job_toggle(const char *id); +/** + * Set the enabled flag to 0 or 1. Unlike cron_job_toggle(), this does not flip + * a row that already has the requested value. + * + * @param id Job id. + * @param enabled 0 to disable, nonzero to enable. + * @return 0 when a row changes, non-zero if the job is missing or already set. + * + * Example: `cron_job_set_enabled("cron_1", 0);` + */ +int cron_job_set_enabled(const char *id, int enabled); + /** * Update next_run for a job. * diff --git a/src/gateway/routes.c b/src/gateway/routes.c index da4e1db..7e604a1 100644 --- a/src/gateway/routes.c +++ b/src/gateway/routes.c @@ -644,7 +644,8 @@ static void handle_cron_create(const char *body, size_t body_len, char *buf, siz agent_unlock(); cJSON_Delete(root); if (ret != 0) { - json_error(buf, size, status, 400, "Failed to create job"); + json_error(buf, size, status, 400, + ret == -2 ? "channel cannot deliver" : "Failed to create job"); return; } cJSON *resp_obj = cJSON_CreateObject(); diff --git a/src/tools/cron.c b/src/tools/cron.c index bf40b15..63515fb 100644 --- a/src/tools/cron.c +++ b/src/tools/cron.c @@ -181,13 +181,39 @@ int cron_is_one_shot(const char *schedule) return strncmp(schedule, CRON_PREFIX_AT, strlen(CRON_PREFIX_AT)) == 0; } +static const char *cron_delivery_channel(const char *channel) +{ + if (!channel || channel[0] == '\0') + return "cli"; + return channel; +} + +/** 1 when a poll can hand this job to a channel that has a send callback. */ +static int cron_target_can_deliver(const char *channel) +{ + const char *name = cron_delivery_channel(channel); + const channel_t *dest; + + if (strcmp(name, "cron") == 0) + return 0; + dest = channel_get_by_name(name); + return dest && dest->send ? 1 : 0; +} + int cron_create_job(const char *schedule, const char *message, const char *channel, const char *recipient, char *id_out, size_t id_size) { + const char *ch; + const char *rec; + if (!schedule || !message || !id_out || id_size == 0) return -1; - const char *ch = channel ? channel : "cli"; - const char *rec = recipient ? recipient : "default"; + ch = channel ? channel : "cli"; + rec = recipient ? recipient : "default"; + /* An unregistered target fails in cron_send after agent_run, and the + * skipped ack leaves next_run in the past so the job reruns forever. */ + if (!cron_target_can_deliver(ch)) + return -2; if (id_out[0] == '\0') { unsigned char rnd[2]; if (crypto_read_urandom(rnd, sizeof(rnd)) != 0) return -1; @@ -382,6 +408,23 @@ static int cron_pick_due_row(long long now, int timeout_ms, cron_job_row_t *row) return 1; } +/** Drop a stored job that can never send, before the agent runs it. */ +static void cron_disable_undeliverable(const cron_job_row_t *row, int timeout_ms) +{ + int rc; + + fprintf(stderr, + "shellclaw: cron: job '%s' channel '%s' cannot deliver; disabling it so a failed send cannot rerun the agent\n", + row->id, cron_delivery_channel(row->channel)); + agent_lock(); + rc = cron_job_set_enabled(row->id, 0); + agent_unlock(); + /* A missed update leaves the row due. Remember the offer so the next poll + * waits instead of spinning the scheduler on the same id. */ + if (rc != 0) + cron_mark_offered(row->id, timeout_ms > 0 ? timeout_ms : 1000); +} + static int cron_poll(channel_incoming_msg_t *out, int timeout_ms) { cron_job_row_t row; @@ -392,6 +435,11 @@ static int cron_poll(channel_incoming_msg_t *out, int timeout_ms) memset(&row, 0, sizeof(row)); if (!cron_pick_due_row(now, timeout_ms, &row)) return 0; + if (!cron_target_can_deliver(row.channel)) { + cron_disable_undeliverable(&row, timeout_ms); + cron_job_row_free(&row); + return 0; + } memset(out, 0, sizeof(*out)); snprintf(session_id, sizeof(session_id), "%s:%s", row.channel[0] ? row.channel : "cli", @@ -553,9 +601,13 @@ static int cron_tool_execute(const char *args_json, char *result_buf, size_t max id[0] = '\0'; const char *ch = (channel && cJSON_IsString(channel)) ? channel->valuestring : NULL; const char *rec = (recipient && cJSON_IsString(recipient)) ? recipient->valuestring : NULL; - if (cron_create_job(schedule->valuestring, message->valuestring, ch, rec, id, sizeof(id)) != 0) { + int created = cron_create_job(schedule->valuestring, message->valuestring, ch, rec, id, + sizeof(id)); + if (created != 0) { cJSON_Delete(root); - snprintf(result_buf, max_len, "{\"error\":\"failed to create job\"}"); + snprintf(result_buf, max_len, "%s", + created == -2 ? "{\"error\":\"channel cannot deliver\"}" + : "{\"error\":\"failed to create job\"}"); return -1; } snprintf(result_buf, max_len, "{\"ok\":true,\"id\":\"%s\"}", id); diff --git a/src/tools/cron.h b/src/tools/cron.h index 803196d..0aa5a06 100644 --- a/src/tools/cron.h +++ b/src/tools/cron.h @@ -59,10 +59,11 @@ const tool_t *tool_cron_get(void); * @param schedule Schedule string (interval:N, at:ts, or cron expression). * @param message Message to inject when job fires. * @param channel Target channel name (e.g., "cli"). NULL defaults to "cli". + * The name must be a registered channel other than "cron". * @param recipient Recipient ID. NULL defaults to "default". * @param id_out Buffer for job ID. If non-empty on entry, used as-is; else generated. * @param id_size Size of id_out buffer. - * @return 0 on success, -1 on error. + * @return 0 on success, -2 if channel cannot deliver, -1 on any other error. */ int cron_create_job(const char *schedule, const char *message, const char *channel, const char *recipient, diff --git a/tests/test_cron.c b/tests/test_cron.c index 5770f68..3431119 100644 --- a/tests/test_cron.c +++ b/tests/test_cron.c @@ -23,6 +23,21 @@ void provider_router_active_backend_snapshot(char *dst, size_t dst_sz) dst[0] = '\0'; } +static int cron_test_deliver(const char *recipient, const char *text, + const channel_attachment_t *attachments, size_t att_count) +{ + (void)recipient; + (void)text; + (void)attachments; + (void)att_count; + return 0; +} + +static const channel_t cron_test_cli_channel = { + .name = "cli", + .send = cron_test_deliver, +}; + static void cron_test_reset_offers(void) { const channel_t *cron_ch = channel_cron_get(); @@ -754,8 +769,58 @@ static int test_cron_poll_delivers_17th_after_ack(void) return 0; } +/** + * A due job whose channel was never registered must not be handed to the agent. + * Send would fail, ack would be skipped, and the still-due row would rerun tools + * on every poll. Direct cron_job_create bypasses the create-time check, which is + * how an already stored row reaches this path. + */ +static int test_unregistered_channel_is_not_delivered(void) +{ + const char *path = "/tmp/shellclaw_test_cron_bad_channel.db"; + const channel_t *cron_ch; + channel_incoming_msg_t msg; + cron_job_row_t row; + long long now; + char id[128]; + + cron_test_reset_offers(); + remove(path); + ASSERT(memory_init(path) == 0); + now = (long long)time(NULL); + ASSERT(cron_job_create("ghost", "interval:60", "run the backup", "no-such-channel", + "default", now - 1, 1) == 0); + cron_ch = channel_cron_get(); + ASSERT(cron_ch != NULL && cron_ch->poll != NULL); + memset(&msg, 0, sizeof(msg)); + ASSERT(cron_ch->poll(&msg, 0) == 0); + ASSERT(msg.text == NULL); + memset(&row, 0, sizeof(row)); + ASSERT(cron_job_get_by_id("ghost", &row) == 1); + ASSERT(row.enabled == 0); + cron_job_row_free(&row); + memset(&msg, 0, sizeof(msg)); + ASSERT(cron_ch->poll(&msg, 0) == 0); + id[0] = '\0'; + ASSERT(cron_create_job("interval:60", "hi", "no-such-channel", "user", id, sizeof(id)) != 0); + ASSERT(cron_job_create("cli_ok", "interval:60", "still delivered", "cli", "default", + now - 1, 1) == 0); + memset(&msg, 0, sizeof(msg)); + ASSERT(cron_ch->poll(&msg, 0) == 1); + ASSERT(msg.text != NULL); + ASSERT(strcmp(msg.text, "still delivered") == 0); + ASSERT(msg.user_id != NULL); + ASSERT(strcmp(msg.user_id, "cli_ok") == 0); + channel_incoming_msg_clear(&msg); + cron_test_reset_offers(); + memory_cleanup(); + remove(path); + return 0; +} + int main(void) { + channel_register("cli", &cron_test_cli_channel); RUN(test_interval_next_run()); RUN(test_at_one_shot()); RUN(test_cron_expr_next_run()); @@ -781,6 +846,7 @@ int main(void) RUN(test_cron_poll_waits_when_offer_table_is_full()); RUN(test_cron_poll_delivers_17th_after_ack()); RUN(test_cron_list_orders_ties_by_id()); + RUN(test_unregistered_channel_is_not_delivered()); printf("test_cron: all tests passed\n"); return 0; }